-
-
Notifications
You must be signed in to change notification settings - Fork 383
Added cppcoreguidelines-init-variables check to .clang-tidy #3009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added cppcoreguidelines-init-variables check to .clang-tidy #3009
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSeeded explicit initializations across many headers and sources (booleans, counters, numerics, loop indices) and added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-12-30T19:45:24.426ZApplied to files:
📚 Learning: 2025-12-30T19:45:40.183ZApplied to files:
📚 Learning: 2025-01-19T13:17:02.243ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds the cppcoreguidelines-init-variables check to the .clang-tidy configuration file to enforce C++ Core Guidelines best practices for variable initialization. The check helps catch uninitialized variable bugs by ensuring all variables are initialized before use.
- Added
cppcoreguidelines-init-variablesto the list of enabled checks - Included explanatory comment documenting the check's purpose
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.clang-tidy(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)
.clang-tidy (1)
9-11: Good addition; aligns with C++ Core Guidelines.The addition of
cppcoreguidelines-init-variableschecks whether there are local variables declared without an initial value, helping to catch cases where code paths might read uninitialized variables. This rule aligns with the Type safety (Type.5) profile and ES.20 from the C++ Core Guidelines. The inline comment clearly explains the purpose, and the formatting is consistent with the existing configuration.
cvvergara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add only the check line, no commenta and then proceed to fix all errors generated by it.
|
If there are no warnings to fix, then look for another one to fix |
…nitialization and removed comment from clang-tidy as per instructions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
.clang-tidyinclude/bdAstar/bdAstar.hppinclude/contraction/contractionGraph.hppinclude/contraction/contractionHierarchies.hppinclude/contraction/linearContraction.hppinclude/cpp_common/base_graph.hppinclude/cpp_common/get_data.hppinclude/lineGraph/lineGraphFull.hppsrc/chinese/chinesePostman_driver.cppsrc/coloring/edgeColoring.cppsrc/common/assert.cppsrc/components/components.cppsrc/cpp_common/compPaths.cppsrc/cpp_common/get_check_data.cppsrc/cpp_common/undirectedNoCostBG.cppsrc/max_flow/max_flow_driver.cppsrc/max_flow/maxflow.cppsrc/max_flow/maximumcardinalitymatching.cppsrc/max_flow/minCostMaxFlow.cppsrc/max_flow/minCostMaxFlow_driver.cppsrc/trsp/trspHandler.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-19T16:18:49.447Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2725
File: src/bellman_ford/bellman_ford_neg_driver.cpp:119-119
Timestamp: 2025-01-19T16:18:49.447Z
Learning: The edge count check in bellman_ford_neg_driver.cpp is part of code that is not yet in use, as of January 2025.
Applied to files:
include/contraction/contractionHierarchies.hppsrc/coloring/edgeColoring.cppsrc/components/components.cppinclude/cpp_common/base_graph.hppsrc/max_flow/minCostMaxFlow.cppsrc/max_flow/maximumcardinalitymatching.cppsrc/cpp_common/undirectedNoCostBG.cppinclude/contraction/linearContraction.hpp
📚 Learning: 2025-01-19T13:17:02.243Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2723
File: tools/developer/run.sh:62-68
Timestamp: 2025-01-19T13:17:02.243Z
Learning: For pgRouting project, CMake configuration comments should be kept minimal and concise, avoiding extensive explanations of build options and configurations.
Applied to files:
.clang-tidy
🧬 Code graph analysis (1)
src/max_flow/maxflow.cpp (1)
include/cpp_common/path.hpp (1)
j(132-132)
🪛 Cppcheck (2.19.0)
src/coloring/edgeColoring.cpp
[style] 87-87: The function 'pop_front' is never used.
(unusedFunction)
src/chinese/chinesePostman_driver.cpp
[style] 82-82: The function 'GetBoostVertex' is never used.
(unusedFunction)
⏰ 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 (35)
.clang-tidy (1)
10-10: LGTM! The check has been added correctly.The
cppcoreguidelines-init-variablescheck is properly added to the configuration, maintaining consistency with the existing cppcoreguidelines checks.src/trsp/trspHandler.cpp (1)
235-235: LGTM! Appropriate initialization for a cost variable.Initializing
totalCosttoNANis correct defensive programming. The variable is assigned fromget_tot_cost()before use in all code paths (lines 247 and 263), and the NAN initialization helps catch potential bugs if the variable were ever used before assignment.src/cpp_common/undirectedNoCostBG.cpp (1)
53-53: LGTM! Standard initialization for boost::tie output parameter.Initializing
addedtofalseis correct practice. Althoughboost::tieon line 55 will set the value before the check on line 56, the initialization ensures defined behavior and aligns with the cppcoreguidelines-init-variables requirement.include/lineGraph/lineGraphFull.hpp (1)
197-197: LGTM! Proper initialization for boost::tie output parameter.Initializing
insertedtofalseis appropriate defensive programming. The variable serves as an output parameter forboost::tieon line 220, and the initialization ensures defined behavior.src/max_flow/maximumcardinalitymatching.cpp (1)
81-81: LGTM! Appropriate initialization for edge existence check.Initializing
existstofalseis correct. The variable is set byboost::tieon line 82 before being checked on line 83, andfalseis a safe default value for an existence check.src/max_flow/minCostMaxFlow.cpp (1)
55-55: LGTM! Initialization for boost::tie output parameter.Initializing
btofalseis appropriate. The variable serves as an output parameter forboost::tieon line 57, and the initialization ensures defined behavior per the cppcoreguidelines-init-variables check.include/contraction/linearContraction.hpp (1)
111-111: LGTM! Proper initialization for edge existence flags.Initializing
found_eandfound_ftofalseis correct. These variables are set byboost::tieon lines 117-118 and 125-126 before being checked in the directed graph case, andfalseis the appropriate default for existence checks.include/cpp_common/get_data.hpp (2)
64-65: LGTM! Explicit initialization ensures defined behavior.Initializing
total_tuplesandvalid_pgtuplesto0is correct. Although these variables are reassigned on line 71, the explicit initialization at declaration aligns with the cppcoreguidelines-init-variables requirement and provides clarity.
123-124: LGTM! Consistent initialization pattern.Initializing
total_tuplesandvalid_pgtuplesto0mirrors the pattern in the first overload and ensures defined behavior. The subsequent reassignment on line 130 doesn't negate the value of explicit initialization at declaration.src/max_flow/minCostMaxFlow_driver.cpp (1)
117-118: LGTM: Appropriate initialization for cost variable.Initializing
min_costtoNANbefore assignment follows the C++ Core Guidelines and provides a clear sentinel value. The variable is immediately assigned on the next line, so there's no risk of using the uninitialized value.src/common/assert.cpp (1)
45-45: LGTM: Explicit initialization of loop counter.Initializing
ito 0 at declaration is good defensive programming practice and aligns with the loop's starting value on line 52.src/cpp_common/compPaths.cpp (1)
58-59: LGTM: Defensive initialization of loop counter.Explicitly initializing
ito 0 is good practice even though the for-loop on line 59 immediately reassigns it. This prevents potential undefined behavior if the code is refactored.include/bdAstar/bdAstar.hpp (1)
159-183: LGTM: Safe default initialization for switch variable.Initializing
currentto 0 ensures defined behavior and aligns with the C++ Core Guidelines. All switch cases assigncurrentbefore the return statement, making this initialization defensive but harmless.src/max_flow/maxflow.cpp (6)
77-77: LGTM: Defensive initialization for Boost output parameter.Initializing
addedtofalsebeforeboost::tieassignment is good defensive practice and aligns with C++ Core Guidelines.
112-112: LGTM: Defensive initialization for Boost output parameter.Initializing
addedtofalsebeforeboost::tieassignment follows the same defensive pattern as other edge insertion methods.
136-136: LGTM: Defensive initialization for Boost output parameter.Consistent initialization pattern for
addedvariable used withboost::tie.
164-164: LGTM: Defensive initialization in supersource setup.Initializing
addedtofalsemaintains consistency with other edge insertion helpers.
184-184: LGTM: Defensive initialization in supersink setup.Consistent initialization pattern for the supersink edge insertion logic.
281-282: LGTM: Proper initialization of loop and output variables.Initializing both
existsandjensures defined values before their use in the path construction logic. The variables are assigned byboost::edge(line 288) and used in the loop before being read.src/max_flow/max_flow_driver.cpp (1)
117-130: LGTM: Safe initialization with comprehensive algorithm dispatch.Initializing
max_flowto 0 is appropriate. All valid algorithm paths (1, 2, 3) assignmax_flowbefore it's used on line 136, and the invalid algorithm case returns an error before any use.src/coloring/edgeColoring.cpp (1)
87-101: LGTM: Consistent initialization pattern for Boost operations.Initializing
addedtofalsebeforeboost::tieon line 101 follows the established pattern across the codebase and aligns with C++ Core Guidelines.src/components/components.cpp (5)
52-56: LGTM: Safe initialization for Boost output parameter.Initializing
num_compsto 0 beforeboost::connected_componentsassignment is good defensive practice and prevents undefined behavior.
77-81: LGTM: Consistent initialization in strongly connected components.Follows the same safe initialization pattern as
pgr_connectedComponents.
111-114: LGTM: Proper initialization for biconnected components count.Consistent defensive initialization before the
biconnected_componentscall.
172-176: LGTM: Safe initialization of initial component count.Initializing
ini_compsto 0 ensures defined behavior before theboost::connected_componentsassignment.
234-238: LGTM: Defensive initialization in bridge detection logic.Initializing
now_compsto 0 in the parallel edge check follows the established pattern and ensures safe usage withboost::connected_components.src/cpp_common/get_check_data.cpp (1)
227-228: LGTM! Variable initializations prevent undefined behavior.The explicit initialization of local variables (
binval,isNull/isnull,typlen,typbyval,typalign) aligns with the C++ Core Guidelines and prevents potential undefined behavior. These variables are assigned immediately after initialization through PostgreSQL API calls, so the defensive initialization doesn't change program behavior.Also applies to: 277-279, 362-364, 454-456, 559-560, 592-592
include/contraction/contractionHierarchies.hpp (3)
132-132: LGTM! Boolean flags properly initialized.Initializing
found_e,found_f, andfound_gtofalsebefore their use withboost::tieis good defensive programming that aligns with C++ Core Guidelines.
173-173: LGTM! Counter initialization ensures defined value.Explicitly initializing
n_old_edgesto 0 ensures a defined value before the conditional assignment in the directed/undirected branches, preventing undefined behavior.
228-228: LGTM! Defensive initialization before computation.Initializing
mto 0 before the computation on lines 229-230 is good defensive practice, even though it's immediately overwritten. This prevents undefined behavior if the code is refactored in the future.src/chinese/chinesePostman_driver.cpp (1)
82-82: LGTM! NAN is an appropriate sentinel value.Initializing
minCosttoNANis a good choice as a sentinel value. If the assignment fromdigraph.DirectedChPP()were to fail (though it shouldn't), the NAN value ensures that the comparisonminCost >= 0.0on line 87 evaluates to false, preventing incorrect execution of the conditional block.include/contraction/contractionGraph.hpp (3)
185-185: LGTM! Boolean initialization before boost::tie.Initializing
insertedtofalsebefore use withboost::tieis good defensive programming.
398-398: LGTM! Boolean flags properly initialized.Initializing
found_eandinsertedtofalsebefore their use ensures defined behavior.Also applies to: 426-426
446-446: LGTM! Loop variable properly initialized.Initializing
found_ftofalsewithin the loop ensures each iteration has a defined starting state.include/cpp_common/base_graph.hpp (1)
712-712: LGTM! Consistent boolean initialization pattern.All boolean variables (
insertedandfound) are properly initialized tofalsebefore use with Boost Graph library functions likeboost::tieandboost::edge. This defensive initialization aligns with C++ Core Guidelines and prevents undefined behavior while maintaining compatibility with the minimum Boost version (1.56.0).Also applies to: 733-733, 764-764, 779-779, 796-796, 819-819
|
I have resolved all the uninitialized variable issues and errors from cppcoreguidelines-init-variables. |
cvvergara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Locally things look ok
I am just waiting for all the CI tests to complete to approve and merge
…blank line added in .clang-tidy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.clang-tidy
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-19T13:17:02.243Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2723
File: tools/developer/run.sh:62-68
Timestamp: 2025-01-19T13:17:02.243Z
Learning: For pgRouting project, CMake configuration comments should be kept minimal and concise, avoiding extensive explanations of build options and configurations.
Applied to files:
.clang-tidy
⏰ 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)
.clang-tidy (1)
9-10: LGTM! Check addition is correct.The comma addition and new
cppcoreguidelines-init-variablescheck are properly formatted and align with the PR objectives to enforce variable initialization before use.
|
As per the instructions given in discourse i enabled the github actions for running the CI workflow ->i pushed .clang-tidy adding a blank line only so that the CI workflow will run and the output is visible in my fork repo. |
cvvergara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining task is to remove the trailing comma
This is just a note:
and also be clean on your commit messages
"as per int instructions" "as per task" what is that?
Here is a summary of commits with comments where applicable and possible rewrites
- added cppcoreguidelines-init-variables check to .clang-tidy as per task 4
- what is task 4? pgRouting do not have a task 4
- Added cppcoreguidelines-init-variables check to .clang-tidy
- clang-tidy: Added cppcoreguidelines-init-variables
- clang-tidy: fixed cppcoreguideline-init-variables and all variables initialization and removed comment from clang-tidy as per instructions
- We (on any project the people that do reviews) always give instructions, that is redundant
- Fix warnings from cppcoreguideline-init-variables check and clang-tidy cleanup
- Fix: Remove redundant initialization of p_max in contractionGraph.hpp
- Fix: Initialize totalCost to 0.0 instead of NAN to resolve Codacy warning
- Fix: Reduce variable scope to resolve Codacy warnings (totalCost, added, found_e)
- Clean commit to Trigger CI - enabled github actions in my fork repo: blank line added in .clang-tidy
- Its clear why you are adding that line that, but you could have created another branch and use that one to solve the
problem you have on your fork. Aka that is a task you do on your fork not on a PR that is to be merged.
- Its clear why you are adding that line that, but you could have created another branch and use that one to solve the
Do not worry about fixing the commit messages I will be doing a squash and will fix the commit messages
By the way you didn't need to fix codacy errors but Thanks for doing those fixes
This PR adds the
cppcoreguidelines-init-variablescheck to the.clang-tidyconfiguration.#What does this check do?
Changes made:
.clang-tidyto includecppcoreguidelines-init-variablescheckSummary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.