clang-tidy: check and fix cppcoreguidelines-pro-type-member-init#3077
clang-tidy: check and fix cppcoreguidelines-pro-type-member-init#3077ayushjariyal wants to merge 6 commits intopgRouting:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnabled clang-tidy check cppcoreguidelines-pro-type-member-init and applied value-initialization ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
include/lineGraph/lineGraphFull.hpp (1)
59-62: 🧹 Nitpick | 🔵 TrivialRedundant explicit initialization of
m_num_edges(0).Now that the default member initializer
m_num_edges{}is in place (line 295), them_num_edges(0)entry in the first constructor's initializer list (line 61) is redundant and can be removed.♻️ Proposed fix
explicit Pgr_lineGraphFull() - : Pgr_base_graph<G, T_V, T_E, t_directed>(), - m_num_edges(0) { + : Pgr_base_graph<G, T_V, T_E, t_directed>() { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/lineGraph/lineGraphFull.hpp` around lines 59 - 62, The Pgr_lineGraphFull() constructor redundantly initializes m_num_edges to 0 even though m_num_edges has a default member initializer; remove m_num_edges(0) from the constructor's initializer list in the explicit Pgr_lineGraphFull() definition so the member default (m_num_edges{}) is used instead and leave the base initializer Pgr_base_graph<G, T_V, T_E, t_directed>() intact.include/chinese/chinesePostman.hpp (1)
79-81:⚠️ Potential issue | 🟠 MajorIncomplete fix — private POD members still lack
{}initializers.
superSource,superTarget,startPoint, andm_costare plain scalar types without default member initializers or initializer-list entries, so thecppcoreguidelines-pro-type-member-initcheck still fires for them. Constructor-body assignment does not satisfy the check.🛠️ Proposed fix
- int64_t superSource, superTarget; - int64_t startPoint; - double m_cost; + int64_t superSource{}, superTarget{}; + int64_t startPoint{}; + double m_cost{};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/chinese/chinesePostman.hpp` around lines 79 - 81, Private POD members superSource, superTarget, startPoint, and m_cost lack in-class default initializers which triggers cppcoreguidelines-pro-type-member-init; add brace initializers to their declarations (e.g., int64_t superSource{};, int64_t superTarget{};, int64_t startPoint{};, double m_cost{};) or alternatively initialize them in the member initializer list of the class constructors (not via constructor-body assignments) to satisfy the guideline and silence the check.src/cpp_common/pgdata_fetchers.cpp (2)
354-354: 🧹 Nitpick | 🔵 TrivialInconsistent initialization style:
= {}vs{}.All other local variables in this file now use the direct brace-init form
T var{}, butfetch_pointstill uses the legacyPoint_on_edge_t point = {};. Both are semantically equivalent, but the inconsistency is worth tidying up.♻️ Proposed fix
- Point_on_edge_t point = {}; + Point_on_edge_t point{};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpp_common/pgdata_fetchers.cpp` at line 354, In fetch_point replace the legacy initialization "Point_on_edge_t point = {};" with the consistent direct brace-init form "Point_on_edge_t point{};" so it matches other locals in this file; locate the declaration in the fetch_point function and update the initialization style for the variable named point to use {} instead of = {}.
337-344: 🧹 Nitpick | 🔵 TrivialRedundant
NULL/0assignments after{}initialization.After
Restriction_t restriction{}, theviapointer andvia_sizefield are already zero-initialized, makingrestriction.via = NULLandrestriction.via_size = 0on lines 340-341 redundant before thegetBigIntArrcall reassigns them on line 342. These assignments are harmless but unnecessary.♻️ Proposed cleanup
Restriction_t restriction{}; restriction.cost = getFloat8(tuple, tupdesc, info[0]); - - restriction.via = NULL; - restriction.via_size = 0; restriction.via = getBigIntArr(tuple, tupdesc, info[1], &restriction.via_size); return restriction;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpp_common/pgdata_fetchers.cpp` around lines 337 - 344, The local Restriction_t restriction is value-initialized with Restriction_t restriction{}, so restriction.via and restriction.via_size are already zero-initialized; remove the redundant assignments restriction.via = NULL and restriction.via_size = 0 before calling getBigIntArr. Keep the initialization of restriction and the subsequent lines that set restriction.cost = getFloat8(...) and restriction.via = getBigIntArr(...); this eliminates unnecessary assignments while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/chinese/chinesePostman.hpp`:
- Line 163: Nit: inconsistent initializer style — change the declaration using
CostFlow_t edge{} to match the existing style used elsewhere (CostFlow_t edge =
{}) so the file consistently uses the same aggregate initializer; locate the
declaration of CostFlow_t edge{} and replace its initializer to = {} to match
the instance at line 188 (symbol: CostFlow_t edge).
In `@include/vrp/tw_node.hpp`:
- Line 229: The doc comment for member m_type is incorrect (it currently reads
"The demand for the Node"); update the comment to describe the node's type
instead—e.g., change the trailing comment on NodeType m_type{}; to something
like "///< The type of the Node (NodeType) indicating its role/category" so it
accurately documents m_type and references the NodeType enum/class.
---
Outside diff comments:
In `@include/chinese/chinesePostman.hpp`:
- Around line 79-81: Private POD members superSource, superTarget, startPoint,
and m_cost lack in-class default initializers which triggers
cppcoreguidelines-pro-type-member-init; add brace initializers to their
declarations (e.g., int64_t superSource{};, int64_t superTarget{};, int64_t
startPoint{};, double m_cost{};) or alternatively initialize them in the member
initializer list of the class constructors (not via constructor-body
assignments) to satisfy the guideline and silence the check.
In `@include/lineGraph/lineGraphFull.hpp`:
- Around line 59-62: The Pgr_lineGraphFull() constructor redundantly initializes
m_num_edges to 0 even though m_num_edges has a default member initializer;
remove m_num_edges(0) from the constructor's initializer list in the explicit
Pgr_lineGraphFull() definition so the member default (m_num_edges{}) is used
instead and leave the base initializer Pgr_base_graph<G, T_V, T_E, t_directed>()
intact.
In `@src/cpp_common/pgdata_fetchers.cpp`:
- Line 354: In fetch_point replace the legacy initialization "Point_on_edge_t
point = {};" with the consistent direct brace-init form "Point_on_edge_t
point{};" so it matches other locals in this file; locate the declaration in the
fetch_point function and update the initialization style for the variable named
point to use {} instead of = {}.
- Around line 337-344: The local Restriction_t restriction is value-initialized
with Restriction_t restriction{}, so restriction.via and restriction.via_size
are already zero-initialized; remove the redundant assignments restriction.via =
NULL and restriction.via_size = 0 before calling getBigIntArr. Keep the
initialization of restriction and the subsequent lines that set restriction.cost
= getFloat8(...) and restriction.via = getBigIntArr(...); this eliminates
unnecessary assignments while preserving behavior.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
.clang-tidyinclude/chinese/chinesePostman.hppinclude/cpp_common/base_graph.hppinclude/cpp_common/ch_edge.hppinclude/cpp_common/ch_vertex.hppinclude/cpp_common/line_vertex.hppinclude/cpp_common/path.hppinclude/lineGraph/lineGraphFull.hppinclude/max_flow/maxflow.hppinclude/max_flow/minCostMaxFlow.hppinclude/spanningTree/mst.hppinclude/trsp/edgeInfo.hppinclude/trsp/trspHandler.hppinclude/vrp/tw_node.hppinclude/yen/turnRestrictedPath.hppsrc/cpp_common/pgdata_fetchers.cppsrc/max_flow/minCostMaxFlow.cpp
💤 Files with no reviewable changes (1)
- .clang-tidy
|
@cvvergara, Could you please review this PR? |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/chinese/chinesePostman.hpp (1)
79-81:⚠️ Potential issue | 🟠 MajorIncomplete fix — scalar data members still lack value initializers.
The PR applies
{}to localCostFlow_t edgevariables but leaves four plain scalar members uninitialised at declaration:int64_t superSource, superTarget; // line 79 — uninitialized int64_t startPoint; // line 80 — uninitialized double m_cost; // line 81 — uninitializedThese are precisely the cases
cppcoreguidelines-pro-type-member-inittargets. They are all assigned in the constructor body before being read, but they pass through an indeterminate state and—crucially—m_costis exposed through the publicDirectedChPP()accessor. None appear in the member-initialiser list (lines 113–117), so a thrown exception during early construction would leave them uninitialised for callers.🛠️ Proposed fix
- int64_t superSource, superTarget; - int64_t startPoint; - double m_cost; + int64_t superSource{}; + int64_t superTarget{}; + int64_t startPoint{}; + double m_cost{-1}; // -1 is the sentinel already used in the constructor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/chinese/chinesePostman.hpp` around lines 79 - 81, The scalar members superSource, superTarget, startPoint and m_cost are left uninitialised at declaration; add default member initializers (e.g. = 0 for the int64_t members and = 0.0 for m_cost) or initialize them in the class constructor member-initializer list for the class containing those fields (referencing the members superSource, superTarget, startPoint, m_cost and the constructor that currently omits them) so they cannot remain indeterminate if construction is aborted early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/vrp/tw_node.hpp`:
- Line 227: The Doxygen comment for member m_service_time is malformed (uses "//
/<") so its documentation is dropped; open the declaration of m_service_time in
tw_node.hpp and change the comment to a proper trailing Doxygen comment (use
"///<" like the other members on lines 224–229) so the service-time description
is picked up by the generated docs and stays consistent with the surrounding
members.
---
Outside diff comments:
In `@include/chinese/chinesePostman.hpp`:
- Around line 79-81: The scalar members superSource, superTarget, startPoint and
m_cost are left uninitialised at declaration; add default member initializers
(e.g. = 0 for the int64_t members and = 0.0 for m_cost) or initialize them in
the class constructor member-initializer list for the class containing those
fields (referencing the members superSource, superTarget, startPoint, m_cost and
the constructor that currently omits them) so they cannot remain indeterminate
if construction is aborted early.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
include/vrp/tw_node.hpp (1)
227-227:⚠️ Potential issue | 🟡 MinorFix malformed Doxygen comment on
m_service_time.
////<is not a valid trailing Doxygen comment, so the documentation won’t be picked up. Use///<like the other members.📝 Proposed fix
- double m_service_time{}; ////< time it takes to be served + double m_service_time{}; ///< time it takes to be served🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/vrp/tw_node.hpp` at line 227, The member variable m_service_time has a malformed Doxygen trailing comment using "////<"; change it to the correct trailing-doc style "///<" so the comment is picked up like the other members (locate the declaration of m_service_time and replace the "////< time it takes to be served" with "///< time it takes to be served").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@include/vrp/tw_node.hpp`:
- Line 227: The member variable m_service_time has a malformed Doxygen trailing
comment using "////<"; change it to the correct trailing-doc style "///<" so the
comment is picked up like the other members (locate the declaration of
m_service_time and replace the "////< time it takes to be served" with "///<
time it takes to be served").
cvvergara
left a comment
cvvergara
left a comment
There was a problem hiding this comment.
I dont see your actions running
|
@cvvergara , I've enabled the actions on my fork and push new commit to trigger them. |
2 participants
Enable
cppcoreguidelines-pro-type-member-initin.clang-tidyfile and fix warning related to it.Summary by CodeRabbit