Fix clang-tidy cppcoreguidelines-use-default-member-init #3006
Conversation
WalkthroughEnabled clang-tidy's cppcoreguidelines-use-default-member-init and migrated several class member initializations from constructor initializer lists to in-class default member initializers across headers and one source file. Changes
Sequence Diagram(s)(no sequence diagrams generated) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.clang-tidy(1 hunks)include/visitors/dijkstra_visitors.hpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
include/visitors/dijkstra_visitors.hpp (1)
192-192: LGTM! In-class initialization aligns with C++ Core Guidelines.The change to use default member initialization for
m_num_examinedis correct and maintains the same semantics as constructor initialization. This aligns with the newly enabledcppcoreguidelines-use-default-member-initcheck.
| cppcoreguidelines-avoid-reference-coroutine-parameters, | ||
| cppcoreguidelines-use-default-member-init, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the .clang-tidy file to confirm the changes
cat -n .clang-tidy | head -20Repository: pgRouting/pgrouting
Length of output: 625
🏁 Script executed:
# Run the verification script from the review comment to find other violations
rg -n -C3 -P ':\s*m_\w+\(0\)|:\s*m_\w+\(nullptr\)|:\s*m_\w+\(false\)|:\s*m_\w+\(true\)' \
--type=cpp --type=h -g '!test/**' -g '!build/**' | head -50Repository: pgRouting/pgrouting
Length of output: 468
🏁 Script executed:
# Check what files were modified in this PR/commit
git diff --name-only HEAD~1 2>/dev/null || git diff --name-only 2>/dev/null || echo "Cannot determine git history"Repository: pgRouting/pgrouting
Length of output: 45
Good addition that enforces modern C++ best practices.
Enabling cppcoreguidelines-use-default-member-init is correct and aligns with C++ Core Guidelines. The trailing comma on line 9 follows best practice for maintainability. Note that the codebase has existing violations of this rule (e.g., in include/cpp_common/path.hpp) that should be addressed to comply with the new check.
🤖 Prompt for AI Agents
In .clang-tidy around lines 9-10, you enabled
cppcoreguidelines-use-default-member-init (with a trailing comma) but the repo
has existing violations (e.g., include/cpp_common/path.hpp); run clang-tidy to
list all violations, then update the affected header/source files by adding
default member initializers on the member declarations (e.g., initialize
pointers, ints, containers to sensible defaults with = {}, = 0, or explicit
values) so members have in-class initializers rather than relying solely on
constructors, and re-run checks to ensure no remaining violations.
Sohamsig
changed the title
Sohamsig
changed the title
cvvergara
left a comment
cvvergara
left a comment
There was a problem hiding this comment.
I compiled locally using:
CXX=clang++ CC=clang cmake -DUSE_CLANG_TIDY=ON -DBUILD_HTML=OFF -DPOSTGRESQL_BIN=${PGBIN} ..
and these are some of the many warnings I get that are not fixed
/home/vicky/pgrouting/pgrouting/cvvergara/include/vrp/solution.hpp:49:13: warning: use default member initializer for 'EPSILON' [cppcoreguidelines-use-default-member-init]
49 | double EPSILON;
| ^
| {0.0001}
[ 16%] Building CXX object src/allpairs/CMakeFiles/allpairs.dir/allpairs_driver.cpp.o
[ 17%] Building CXX object src/withPoints/CMakeFiles/withPoints.dir/withPoints.cpp.o
[ 18%] Building CXX object src/pickDeliver/CMakeFiles/pickDeliver.dir/fleet.cpp.o
/home/vicky/pgrouting/pgrouting/cvvergara/include/vrp/solution.hpp:49:13: warning: use default member initializer for 'EPSILON' [cppcoreguidelines-use-default-member-init]
49 | double EPSILON;
| ^
| {0.0001}
[ 18%] Building CXX object src/pickDeliver/CMakeFiles/pickDeliver.dir/pickDeliver.cpp.o
/home/vicky/pgrouting/pgrouting/cvvergara/include/contraction/contractionGraph.hpp:552:13: warning: use default member initializer for 'min_edge_id' [cppcoreguidelines-use-default-member-init]
70 | int64_t min_edge_id;
| ^
| {0}
/home/vicky/pgrouting/pgrouting/cvvergara/include/bdAstar/bdAstar.hpp:187:9: warning: use default member initializer for 'm_heuristic' [cppcoreguidelines-use-default-member-init]
76 | int m_heuristic;
| ^
| {5}
/home/vicky/pgrouting/pgrouting/cvvergara/include/bdAstar/bdAstar.hpp:188:12: warning: use default member initializer for 'm_factor' [cppcoreguidelines-use-default-member-init]
77 | double m_factor;
| ^
| {1.0}
/home/vicky/pgrouting/pgrouting/cvvergara/include/cpp_common/bidirectional.hpp:225:12: warning: use default member initializer for 'best_cost' [cppcoreguidelines-use-default-member-init]
77 | double best_cost;
| ^
| {0}
/
Fix all
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/chinese/chinesePostman.hpp (1)
178-178: Consider removing redundant assignment.The assignment
totalDeg = 0is redundant sincetotalDegis already initialized to0via the in-class default initializer at line 73. This assignment can be safely removed.🔎 Proposed fix
std::map<int64_t, int>::iterator iter; - totalDeg = 0; for (iter = deg.begin(); iter != deg.end(); ++iter) {include/vrp/solution.hpp (1)
75-80: Fix: Replace comma operator with semicolon.Line 76 uses a comma operator (
,) instead of a semicolon (;) after theEPSILON = 0.0001assignment. While the comma operator is technically valid C++, this appears to be a typo and makes the code confusing. Use a semicolon to properly separate statements.🔎 Proposed fix
Solution& operator = (const Solution& sol) { - EPSILON = 0.0001, + EPSILON = 0.0001; fleet = sol.fleet; trucks = sol.trucks; return *this; };
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
include/chinese/chinesePostman.hppinclude/contraction/contractionGraph.hppinclude/vrp/solution.hppinclude/yen/ksp.hppsrc/pickDeliver/solution.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
include/contraction/contractionGraph.hpp (1)
551-551: LGTM! In-class default initializer added correctly.The in-class default initializer for
min_edge_id = 0correctly replaces the constructor initializer list initialization, aligning with the clang-tidy rulecppcoreguidelines-use-default-member-init.src/pickDeliver/solution.cpp (1)
232-234: LGTM! Correctly removes EPSILON from constructor initializer list.The removal of
EPSILON(0.0001)from the constructor initializer list is correct, asEPSILONnow has an in-class default initializer ininclude/vrp/solution.hpp.include/chinese/chinesePostman.hpp (1)
73-74: LGTM! In-class default initializers added correctly.The in-class default initializers
totalDeg{0}andtotalCost{0.0}correctly replace constructor initializer list initialization, aligning with the clang-tidy rule.include/vrp/solution.hpp (2)
49-49: LGTM! In-class default initializer added correctly.The in-class default initializer
EPSILON = 0.0001correctly replaces constructor initializer list initialization, aligning with the clang-tidy rule.
68-72: LGTM! Copy constructor correctly omits EPSILON from initializer list.The copy constructor correctly relies on the in-class default initializer for
EPSILONinstead of explicitly initializing it in the initializer list.include/yen/ksp.hpp (2)
61-64: LGTM! Constructor correctly simplified.The constructor correctly relies on in-class default initializers for member variables, only performing explicit initialization for
m_vis.
229-232: LGTM! In-class default initializers added correctly.The in-class default initializers for
m_start{0},m_end{0},m_K{0}, andm_heap_paths{false}correctly replace constructor initializer list initialization, aligning with the clang-tidy rulecppcoreguidelines-use-default-member-init.
|
I've fixed remaining clang-tidy warnings for cppcoreguidelines-use-default-member-init by replacing redundant constructor initializations with default member initializers. Build, clang-tidy, and docs all verified locally. |
include/vrp/solution.hpp
Outdated
| /* @brief copy constructor */ | ||
| Solution(const Solution &sol) : | ||
| EPSILON(0.0001), | ||
|
|
There was a problem hiding this comment.
Style check is failing because of this white line.
Remove the line
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
include/vrp/solution.hpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
include/vrp/solution.hpp (1)
68-78: EPSILON is never modified in the codebase and appears to be unused—consider removing it or making it static constexpr.The copy constructor and copy assignment operator do not copy the
EPSILONmember. However, this is not a copy semantics bug: EPSILON is never modified or read anywhere in the codebase, appearing only at its declaration (line 49). Every Solution object will have EPSILON = 0.0001 regardless of whether it is copied.If EPSILON is intended to be a constant, it should be declared as
static constexpr double EPSILON = 0.0001;rather than an instance member with a default initializer. If it is not used, it should be removed.
| friend class PD_problem; | ||
| protected: | ||
| double EPSILON; | ||
| double EPSILON = 0.0001; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making EPSILON static constexpr if it's never modified.
If EPSILON is a constant tolerance value that's never modified after initialization, it should be declared as:
static constexpr double EPSILON = 0.0001;This would make the constant nature explicit, save memory (one shared value instead of per-instance), enable compile-time evaluation, and align with modern C++ best practices.
However, if verification reveals that EPSILON can be modified per instance, then the copy semantics issue flagged in the previous comment must be addressed instead.
🤖 Prompt for AI Agents
In include/vrp/solution.hpp around line 49, EPSILON is currently a non-const
member; change it to a compile-time constant by declaring it as static constexpr
double EPSILON = 0.0001; (either as a class static constexpr or a
header/namespace-level constexpr) and remove the per-instance storage; update
all uses to refer to the static/namespace constant (e.g. Solution::EPSILON or
vrp::EPSILON) and remove any code that mutates EPSILON; if EPSILON must be
instance-modifiable, instead leave it non-static and address the earlier
copy-semantics concern instead.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.