clang-tidy: add cppcoreguidelines-special-member-functions #3021
Conversation
WalkthroughAdjusts special-member-function declarations across several headers: defaulting or deleting copy/move constructors and assignment operators, and defaulting destructors for exception, vertex, message, fleet, and problem classes; also updates .clang-tidy to enable cppcoreguidelines-special-member-functions. Changes
Sequence Diagram(s)(omitted — changes are API/member-signature adjustments without multi-component runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 (3)
🧰 Additional context used🧠 Learnings (18)📚 Learning: 2025-12-30T19:45:24.426ZApplied to files:
📚 Learning: 2025-12-30T19:45:40.183ZApplied to files:
📚 Learning: 2025-12-31T06:01:45.144ZApplied to files:
📚 Learning: 2025-02-27T16:32:00.573ZApplied to files:
📚 Learning: 2025-02-27T16:31:49.972ZApplied to files:
📚 Learning: 2025-02-27T23:03:08.160ZApplied to files:
📚 Learning: 2025-10-30T16:43:11.554ZApplied to files:
📚 Learning: 2025-04-24T23:36:04.580ZApplied to files:
📚 Learning: 2025-04-22T22:41:25.296ZApplied to files:
📚 Learning: 2025-04-24T23:34:23.099ZApplied to files:
📚 Learning: 2025-04-22T22:59:10.377ZApplied to files:
📚 Learning: 2025-04-22T22:36:50.711ZApplied to files:
📚 Learning: 2025-04-22T22:58:08.854ZApplied to files:
📚 Learning: 2025-09-14T23:32:11.977ZApplied to files:
📚 Learning: 2025-04-22T22:58:42.373ZApplied to files:
📚 Learning: 2025-04-22T22:41:34.709ZApplied to files:
📚 Learning: 2025-09-15T00:39:05.272ZApplied to files:
📚 Learning: 2025-05-22T21:14:00.661ZApplied to files:
🔇 Additional comments (6)
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
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
include/vrp/solution.hpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:24.426Z
Learning: In pgRouting's .clang-tidy file (and similar list configurations), the last line of lists should not have a trailing comma, per project convention.
🧬 Code graph analysis (1)
include/vrp/solution.hpp (1)
src/pickDeliver/solution.cpp (1)
Solution(232-240)
⏰ 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)
67-69: The defaulted copy operations will copy EPSILON from the source object rather than resetting it to 0.0001, but this behavioral change has no practical impact. EPSILON is never read, compared, or used in any logic within the codebase—it only appears in the member declaration and constructor initialization. Since the member is unused, whether it's copied or reset is inconsequential.Likely an incorrect or invalid review comment.
mayurgalhate
force-pushed
the
clang-tidy-special-member-functions
branch
from
b0b4670 to
f3553dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/cpp_common/line_vertex.hpp (1)
58-75: Consider simplifying: all members are trivially copyable.Since all members (
int64_t,double) are trivially copyable, both copy operations could be defaulted:🔎 Simplified implementation
- Line_vertex(const Line_vertex &v) : - id(v.id), - vertex_id(v.vertex_id), - source(v.source), - target(v.target), - cost(v.cost) {} - Line_vertex& operator=(const Line_vertex &v) { - cp_members(v); - return *this; - } + Line_vertex(const Line_vertex&) = default; + Line_vertex& operator=(const Line_vertex&) = default;This also allows removing the
cp_members(const Line_vertex&)overload if it's not used elsewhere.
♻️ Duplicate comments (1)
include/vrp/solution.hpp (1)
64-72: Addnoexceptto move operations as previously noted.The past review correctly identified that move operations should be
noexceptfor optimal container performance. All member types (double,std::deque,Fleet) support noexcept moves.🔎 Proposed fix
- Solution(Solution&&) = default; - Solution& operator=(Solution&&) = default; + Solution(Solution&&) noexcept = default; + Solution& operator=(Solution&&) noexcept = default;
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
include/bdAstar/bdAstar.hppinclude/bdDijkstra/bdDijkstra.hppinclude/chinese/chinesePostman.hppinclude/cpp_common/assert.hppinclude/cpp_common/basic_vertex.hppinclude/cpp_common/bidirectional.hppinclude/cpp_common/line_vertex.hppinclude/cpp_common/messages.hppinclude/trsp/trspHandler.hppinclude/vrp/fleet.hppinclude/vrp/pd_problem.hppinclude/vrp/solution.hppinclude/withPoints/withPoints.hppinclude/yen/ksp.hpp
🧰 Additional context used
🧬 Code graph analysis (3)
include/vrp/solution.hpp (1)
src/pickDeliver/solution.cpp (1)
Solution(232-240)
include/vrp/fleet.hpp (1)
src/pickDeliver/fleet.cpp (2)
Fleet(50-54)Fleet(56-63)
include/vrp/pd_problem.hpp (1)
src/pickDeliver/pd_problem.cpp (1)
PD_problem(40-47)
⏰ 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 (9)
include/cpp_common/basic_vertex.hpp (1)
56-58: LGTM! Completes the Rule of Five appropriately.The defaulted move operations and destructor complete the special member function set for this simple class. Since
Basic_vertexcontains only a trivialint64_tmember, all defaulted operations are safe and efficient.include/trsp/trspHandler.hpp (1)
118-122: LGTM! Explicit non-copyable/non-movable semantics.While the base class
Pgr_messages(line 54) already prevents copy and move operations, explicitly deleting them in the derived class makes the intent clear and serves as better API documentation.include/bdAstar/bdAstar.hpp (1)
83-86: Verify that the base classPgr_bidirectional<G>supports copy and move operations.Similar to
Pgr_bdDijkstra, this class defaults copy and move operations while inheriting fromPgr_bidirectional<G>(line 47). The additional membersm_heuristicandm_factor(lines 192-193) are trivially copyable, but the base class's copyability still needs verification.Ensure the base class safely supports copy/move operations before relying on defaulted behavior.
The verification script from the
bdDijkstra.hppreview applies equally here, as both classes share the same base class.include/cpp_common/messages.hpp (1)
43-47: Remove this comment — the move operation deletion is intentional design.
Pgr_messagesis designed as a reference-only class. All usage patterns in the codebase return references (Pgr_messages&), never values. The deletion of both copy and move operations enforces this reference-based API and prevents accidental value semantics. This is consistent with the class's role as a shared message accumulator accessed through static or instance methods. No code attempts to move instances or store them in containers, confirming the design intent is correct.Likely an incorrect or invalid review comment.
include/bdDijkstra/bdDijkstra.hpp (1)
82-85: The base classPgr_bidirectional<G>explicitly defaults copy and move operations, making the derived class defaults safe.The base class (bidirectional.hpp lines 83-86) provides matching default copy/move operations. The mutable algorithm state members (priority queues and vectors) employ proper C++ deep-copy semantics via
std::vector's copy constructor, so each copy maintains independent state rather than sharing. The reference memberG &graphis correctly copied as a reference.In practice, the object is instantiated once in the driver code and reused across iterations using
clear()to reset state. Copy/move operations are never invoked. Consider whether these defaults should be explicitly deleted to prevent accidental copies and clarify intent.include/withPoints/withPoints.hpp (1)
51-58: LGTM: Proper Rule of Five implementation.The class correctly declares all five special member functions, making
Pg_points_graphexplicitly non-copyable and non-movable while defaulting the destructor. This satisfies thecppcoreguidelines-special-member-functionscheck.include/vrp/fleet.hpp (1)
65-67: LGTM: Rule of Five with propernoexceptmove operations.Good implementation. The
noexceptspecifiers on move operations enable optimal container performance.Minor: The indentation is inconsistent with the rest of the class (missing leading spaces).
include/vrp/pd_problem.hpp (1)
48-60: LGTM: Proper non-copyable/non-movable semantics.The class manages static problem pointers across multiple related classes, making non-copyable/non-movable semantics the correct choice. All five special member functions are explicitly declared, satisfying the Rule of Five.
include/yen/ksp.hpp (1)
71-74: LGTM! Appropriate Rule of 5 compliance.Deleting copy/move operations is correct here since
Pgr_kspmanages a raw pointer (m_vis) with manual new/delete. This prevents double-delete bugs from accidental copies.
mayurgalhate
force-pushed
the
clang-tidy-special-member-functions
branch
2 times, most recently
from
6d85782 to
467fd8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @include/bdAstar/bdAstar.hpp:
- Around line 83-86: Pgr_bdAstar is declaring defaulted copy/move special
members while its base class Pgr_bidirectional explicitly deletes them, which
makes these defaults implicitly deleted; remove the four lines that default the
copy constructor, copy assignment, move constructor, and move assignment for
Pgr_bdAstar (the declarations referencing Pgr_bdAstar(const Pgr_bdAstar&),
Pgr_bdAstar& operator=(const Pgr_bdAstar&), Pgr_bdAstar(Pgr_bdAstar&&), and
Pgr_bdAstar& operator=(Pgr_bdAstar&&)) so the class relies on the base’s deleted
semantics, or alternatively decide to enable copying/moving by modifying
Pgr_bidirectional accordingly if intended.
In @include/bdDijkstra/bdDijkstra.hpp:
- Around line 82-86: The class Pgr_bdDijkstra currently declares copy/move ctors
and assignment operators as = default but its base Pgr_bidirectional<G>
explicitly deletes them, so these will be implicitly deleted; change the
declarations in Pgr_bdDijkstra to explicitly delete the copy constructor, copy
assignment, move constructor and move assignment (mark each with = delete) to
match the base class semantics and avoid confusing/defaulted declarations
referencing Pgr_bdDijkstra(const Pgr_bdDijkstra&), Pgr_bdDijkstra&
operator=(const Pgr_bdDijkstra&), Pgr_bdDijkstra(Pgr_bdDijkstra&&) and
Pgr_bdDijkstra& operator=(Pgr_bdDijkstra&&).
In @include/cpp_common/line_vertex.hpp:
- Around line 64-67: The copy assignment operator for Line_vertex should be
defaulted instead of manually calling cp_members; replace the current
Line_vertex& operator=(const Line_vertex &v) implementation with a defaulted
declaration (use Line_vertex& operator=(const Line_vertex&) = default;) so the
compiler-generated trivial assignment is used, keeping consistency with the copy
constructor and avoiding the manual cp_members call.
- Around line 74-75: The class Line_vertex unnecessarily deletes move
operations; since its members are trivially copyable (int64_t, double), restore
move semantics by replacing the deleted move constructor and move assignment
with defaulted ones (or remove those declarations to let the compiler generate
them): change Line_vertex(Line_vertex&&) = delete; and Line_vertex&
operator=(Line_vertex&&) = delete; to Line_vertex(Line_vertex&&) = default; and
Line_vertex& operator=(Line_vertex&&) = default; so containers can move
Line_vertex efficiently.
In @include/vrp/fleet.hpp:
- Around line 65-68: Align the indentation of the three special-member
declarations (~Fleet(), Fleet(Fleet&&) noexcept, and Fleet& operator=(Fleet&&)
noexcept) to match surrounding class member indentation (use the same
column/spacing as other members) so they are consistently formatted with the
rest of the class; update whitespace only—no semantic changes.
♻️ Duplicate comments (3)
include/vrp/solution.hpp (1)
64-72: Defaulted special member functions are appropriate for this class.The class members (
EPSILON,fleet,trucks) are all copyable and movable, making defaulted operations valid. The static members don't affect copy/move semantics.Consider adding
noexceptto move operations for optimalstd::vectorreallocation performance, as noted in previous review.include/chinese/chinesePostman.hpp (1)
64-68: Correctly deletes copy/move operations for resource-managing class.The class manages resources (raw pointers in
edgeToIdxpointing intooriginalEdges) and has a user-defined destructor. Deleting copy/move operations prevents undefined behavior from shallow copies.The trailing whitespace on line 64 and inconsistent indentation were noted in previous review.
include/yen/ksp.hpp (1)
131-135: Fix inconsistent indentation (duplicate issue).Lines 131-134 use 5 spaces of indentation while surrounding member declarations (lines 128-129) use 10 spaces. Adjust these lines to use 10 spaces for consistency.
🔎 Proposed fix for consistent indentation
class Visitor { public: Visitor() = default; virtual ~Visitor() {} - Visitor(const Visitor&) = delete; - Visitor& operator=(const Visitor&) = delete; - Visitor(Visitor&&) = delete; - Visitor& operator=(Visitor&&) = delete; - + Visitor(const Visitor&) = delete; + Visitor& operator=(const Visitor&) = delete; + Visitor(Visitor&&) = delete; + Visitor& operator=(Visitor&&) = delete; + virtual void on_insert_first_solution(const Path) const {
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
include/bdAstar/bdAstar.hppinclude/bdDijkstra/bdDijkstra.hppinclude/chinese/chinesePostman.hppinclude/cpp_common/assert.hppinclude/cpp_common/basic_vertex.hppinclude/cpp_common/bidirectional.hppinclude/cpp_common/line_vertex.hppinclude/cpp_common/messages.hppinclude/trsp/trspHandler.hppinclude/vrp/fleet.hppinclude/vrp/pd_problem.hppinclude/vrp/solution.hppinclude/withPoints/withPoints.hppinclude/yen/ksp.hpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:32.986Z
Learning: In pgRouting's .clang-tidy file (and similar list configurations), the last line of lists should not have a trailing comma, per project convention.
📚 Learning: 2025-12-30T19:45:32.986Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:32.986Z
Learning: In pgRouting's .clang-tidy file (and similar list configurations), the last line of lists should not have a trailing comma, per project convention.
Applied to files:
include/chinese/chinesePostman.hpp
🧬 Code graph analysis (3)
include/vrp/pd_problem.hpp (1)
src/pickDeliver/pd_problem.cpp (1)
PD_problem(40-47)
include/vrp/fleet.hpp (1)
src/pickDeliver/fleet.cpp (2)
Fleet(50-54)Fleet(56-63)
include/vrp/solution.hpp (1)
src/pickDeliver/solution.cpp (1)
Solution(232-240)
⏰ 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 (9)
include/cpp_common/messages.hpp (1)
43-47: Special member functions correctly declared for non-transferable design.The explicit deletion of move semantics (along with the already-deleted copy semantics) makes
Pgr_messagesnon-copyable and non-movable. Whilestd::ostringstreammembers are movable, deleting move operations here appears to be a deliberate design choice to prevent message state transfer. The defaulted destructor is appropriate.include/cpp_common/basic_vertex.hpp (1)
56-58: LGTM! Rule of Five completed appropriately.The defaulted move constructor, move assignment operator, and destructor appropriately complete the special member functions for this class. Since the class contains only a trivial
int64_tmember, defaulting these operations is the correct choice.include/withPoints/withPoints.hpp (1)
53-58: LGTM! Special member functions consistent with non-copyable/non-movable base class.The deleted copy/move operations are required since the base class
Pgr_messagesis non-copyable and non-movable. The defaulted destructor is appropriate. These declarations correctly enforce the non-transferable design inherited from the base class.include/vrp/pd_problem.hpp (1)
48-60: LGTM! Non-transferable design appropriate for problem state manager.The deleted copy/move operations are correct for this class. Based on the constructor implementation (which sets static problem pointers across multiple classes:
Dnode,Fleet,Solution,Tw_node,PD_Orders, andVehicle), copying or moving would create conflicts with shared static state. The non-transferable design enforces single-ownership semantics for the problem state.include/cpp_common/bidirectional.hpp (1)
82-87: Correctly deletes special member functions.The class has a reference member (
G &graph) and a non-copyablestd::ostringstreammember, making copy/move operations impossible. Explicitly deleting them clarifies intent and satisfies the clang-tidy rule, addressing the concern from the previous review.include/trsp/trspHandler.hpp (1)
118-122: Appropriate deletion of copy/move for handler class.
TrspHandlermanages complex algorithm state (multiple containers, maps, priority queues). Deleting copy/move operations prevents accidental copies and enforces single-ownership semantics, which is appropriate for handler/controller classes.include/cpp_common/assert.hpp (2)
147-148: LGTM! Good modernization of exception specification.The changes from
throw()tonoexceptand addingoverrideare best practice improvements that modernize the code while maintaining the same semantics.
150-153: Previous critical issue resolved—exception is now throwable.The copy and move constructors are now defaulted (not deleted), which correctly allows the exception to be thrown. Deleting the assignment operators is appropriate for exception types.
include/yen/ksp.hpp (1)
71-74: LGTM! Correct special member function declarations for manual memory management.Deleting copy and move operations is appropriate given that
Pgr_kspmanages a raw pointer (m_vis) with manual allocation/deallocation. This prevents double-free errors.
mayurgalhate
force-pushed
the
clang-tidy-special-member-functions
branch
from
467fd8a to
734c558
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @include/bdAstar/bdAstar.hpp:
- Around line 83-86: The four defaulted special members in Pgr_bdAstar
(Pgr_bdAstar(const Pgr_bdAstar&), operator=(const Pgr_bdAstar&),
Pgr_bdAstar(Pgr_bdAstar&&), operator=(Pgr_bdAstar&&)) are invalid because the
base class Pgr_bidirectional has those operations deleted; remove these
defaulted declarations from Pgr_bdAstar or explicitly declare them deleted to
match the base class (i.e., either delete the copy/move ctor/assign in
Pgr_bdAstar or simply remove those lines so the base-class-deleted semantics
apply).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/cpp_common/assert.hpp (1)
142-161: Critical: Duplicate class member definitions will cause compilation failure.The class contains duplicate definitions of all members:
const std::string str;appears at both line 143 and line 155what(), constructor, and destructor are declared twice (lines 146-148 vs 158-160)The old member declarations (lines 154-161) were not removed when adding the new special member functions. This code will not compile.
🔎 Proposed fix: Remove duplicate declarations
AssertFailedException(const AssertFailedException&) = default; AssertFailedException(AssertFailedException&&) = default; AssertFailedException& operator=(const AssertFailedException&) = delete; // cannot assign const member AssertFailedException& operator=(AssertFailedException&&) = delete; // cannot assign const member - private: - const std::string str; ///< Holds what() we got as message - - public: - const char *what() const noexcept override; - explicit AssertFailedException(std::string msg); - ~AssertFailedException() noexcept override {} };
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
include/bdAstar/bdAstar.hppinclude/cpp_common/assert.hpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:32.986Z
Learning: In pgRouting's .clang-tidy file (and similar list configurations), the last line of lists should not have a trailing comma, per project convention.
📚 Learning: 2025-04-25T00:59:52.580Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2868
File: src/contraction/contractionHierarchies_driver.cpp:47-56
Timestamp: 2025-04-25T00:59:52.580Z
Learning: In pgRouting, driver functions like `pgr_contractionHierarchies` assume that callers within the project will pass valid non-null pointers, as these functions are only used internally with well-defined call patterns. Null checks on the pointers themselves (as opposed to their dereferenced values) are considered unnecessary since the caller is guaranteed to pass references to variables.
Applied to files:
include/bdAstar/bdAstar.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 (2)
include/cpp_common/assert.hpp (2)
150-151: Excellent fix for the copy/move constructors.Defaulting the copy and move constructors correctly addresses the previous critical issue. Exception types must be copyable for
throwto work, and this implementation now satisfies that requirement while maintaining the rule-of-five compliance flagged by clang-tidy.
152-153: Assignment operators correctly deleted.Deleting the assignment operators is appropriate since the class contains a
constmember (str), which makes assignment impossible anyway. The inline comments clearly document this rationale.
mayurgalhate
force-pushed
the
clang-tidy-special-member-functions
branch
2 times, most recently
from
734c558 to
b9bd496
Compare
cvvergara
left a comment
cvvergara
left a comment
There was a problem hiding this comment.
I am going to reduce the scope of your PR
Because you are adding so many new default constructors.
From the clang-tidy documentation cppcoreguidelines-special-member-functions have 4 options that have default false.
Do the .clang-tidy modification to add
cppcoreguidelines-special-member-functions
and the 4 options but with value true
You will need to restore the files that you modified
git fetch upstream
git checkout upstream/develop path/to/file
in each of the files that you modified under include
What is the command to find out the names of the files that are different from upstream/develop?
Do the fixes again, there will be less lines that you add.
and commit, the fixes and the clang tidy file
Which is not modified here.
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 (1)
include/cpp_common/basic_vertex.hpp (1)
48-49: Consider defaulting the copy constructor for consistency.The explicit copy constructor performs the same operation as the default copy constructor would (copying the
int64_t idmember). Since the copy assignment operator (line 54) and other special members are already defaulted, consider defaulting this constructor as well for consistency.🔎 Proposed refactor
- Basic_vertex(const Basic_vertex &v) : - id(v.id) {} + Basic_vertex(const Basic_vertex&) = default;
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.clang-tidyinclude/cpp_common/assert.hppinclude/cpp_common/basic_vertex.hppinclude/cpp_common/messages.hppinclude/vrp/fleet.hppinclude/vrp/pd_problem.hpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-30T19:45:24.426Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:24.426Z
Learning: In .clang-tidy files (and similar list-based configuration files), the last entry in a list should not have a trailing comma, following project convention. Enforce that lists do not end with a comma to improve diffs and consistency across configuration files.
Applied to files:
.clang-tidy
📚 Learning: 2025-12-30T19:45:40.183Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:11-11
Timestamp: 2025-12-30T19:45:40.183Z
Learning: In any .clang-tidy configuration files, ensure that the last element of a list does not have a trailing comma, to follow the repository's coding convention. This applies to all pgRouting/.clang-tidy files across the codebase.
Applied to files:
.clang-tidy
🧬 Code graph analysis (2)
include/vrp/fleet.hpp (1)
src/pickDeliver/fleet.cpp (2)
Fleet(50-54)Fleet(56-63)
include/vrp/pd_problem.hpp (1)
src/pickDeliver/pd_problem.cpp (1)
PD_problem(40-47)
⏰ 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)
.clang-tidy (2)
2-11: LGTM! Configuration correctly enables the new check.The addition of
cppcoreguidelines-special-member-functionsto the checks list is correctly formatted with no trailing comma on the last entry, following the repository's convention.
13-23: LGTM! Comprehensive check options.All sub-checks for special member functions are appropriately enabled, ensuring thorough enforcement of the C++ Rule of Five across the codebase.
include/cpp_common/messages.hpp (1)
43-45: LGTM! Special member functions correctly align with non-copyable members.The defaulted destructor and deleted move operations are appropriate since the class contains
mutable std::ostringstreammembers, which are neither copyable nor movable. The class is now explicitly non-copyable and non-movable, making the design intent clear.include/vrp/fleet.hpp (1)
65-68: LGTM! Correctly completes the Rule of Five.The defaulted move operations (correctly marked
noexcept) and destructor properly complement the existing user-defined copy constructor at src/pickDeliver/fleet.cpp:49-53. This makes the class's copy/move semantics explicit and enables efficient move operations with standard containers.include/cpp_common/assert.hpp (1)
148-152: LGTM! Exception semantics correctly implemented.The defaulted copy and move constructors satisfy the C++ requirement that exception types be copy-constructible for
throwoperations (lines 99, 123). Deleted assignment operators are appropriate since theconst std::string strmember prevents assignment anyway. Thenoexceptspecifications on move operations follow best practices.include/vrp/pd_problem.hpp (1)
48-53: LGTM! Non-copyable, non-movable design is appropriate.Deleting all copy and move operations is correct for this class, which manages global state by setting static problem pointers across multiple classes (see src/pickDeliver/pd_problem.cpp:39-46). The explicit deletions prevent accidental copying or moving that could corrupt the shared state model.
include/cpp_common/basic_vertex.hpp (1)
55-57: LGTM! Rule of Five correctly implemented.The added special member functions (destructor, move constructor, and move assignment operator) are appropriately defaulted, completing the Rule of Five for this class. Since
Basic_vertexcontains only a trivial member (int64_t id), defaulting these operations is correct and ensures the class follows C++ Core Guidelines.
|
I have restored all modified files from upstream/develop, updated the .clang-tidy to enable cppcoreguidelines-special-member-functions with all 4 options set to true, and reapplied the fixes. The changes are now minimal. |
…et, Solution, and common classes
|
Resolved linter errors for spacing/indentation in messages.hpp, pd_problem.hpp, and basic_vertex.hpp |
cvvergara
left a comment
cvvergara
left a comment
There was a problem hiding this comment.
The alignments are wrong.
- Please make sure you are using your .editorconfig appropriately
- Do not modify exisitng code/coments: that includes changing alignment
Please install meld and use
git difftool -t meld -y upstream/develop
And make your PR with the least amount of changes
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @.clang-tidy:
- Line 27: Revert the spacing change on the "FormatStyle" entry in .clang-tidy
so it matches the repository's existing YAML formatting (restore the previous
multiple-space gap between the key "FormatStyle" and its value "none" or
otherwise match the prior commit's exact spacing) to avoid unnecessary churn; do
this by editing the "FormatStyle" line back to the original spacing used in the
file.
In @include/vrp/pd_problem.hpp:
- Line 46: Fix the typo in the comment that reads "Not wllowing to copy the
problem" by updating it to a clearer, correct wording such as "Not allowing to
copy the problem" or better "Disallow copying of the problem" in
include/vrp/pd_problem.hpp so the comment correctly conveys the intent of the
copy-prevention declaration.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.clang-tidyinclude/cpp_common/assert.hppinclude/cpp_common/basic_vertex.hppinclude/cpp_common/messages.hppinclude/vrp/fleet.hppinclude/vrp/pd_problem.hpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-30T19:45:24.426Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:24.426Z
Learning: In .clang-tidy files (and similar list-based configuration files), the last entry in a list should not have a trailing comma, following project convention. Enforce that lists do not end with a comma to improve diffs and consistency across configuration files.
Applied to files:
.clang-tidy
📚 Learning: 2025-12-30T19:45:40.183Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:11-11
Timestamp: 2025-12-30T19:45:40.183Z
Learning: In any .clang-tidy configuration files, ensure that the last element of a list does not have a trailing comma, to follow the repository's coding convention. This applies to all pgRouting/.clang-tidy files across the codebase.
Applied to files:
.clang-tidy
🧬 Code graph analysis (2)
include/vrp/fleet.hpp (1)
src/pickDeliver/fleet.cpp (2)
Fleet(50-54)Fleet(56-63)
include/vrp/pd_problem.hpp (1)
src/pickDeliver/pd_problem.cpp (1)
PD_problem(40-47)
⏰ 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 (6)
include/cpp_common/basic_vertex.hpp (1)
55-57: LGTM - Move semantics properly enabled.The defaulted move constructor, move assignment, and destructor complete the special member function set for this class. Since
Basic_vertexcontains only a trivially copyableint64_tmember, the defaulted move operations are safe and efficient.include/vrp/fleet.hpp (1)
65-67: LGTM - Move semantics intentionally disabled.The explicit deletion of move operations while retaining copy semantics is appropriate for this fleet management class. This design prevents accidental moves of the vehicle collection while allowing explicit copies when needed.
.clang-tidy (1)
10-23: LGTM - Special member function checks properly configured.The addition of
cppcoreguidelines-special-member-functionswith all five options enabled (DefaultConstructor, CopyConstructor, CopyAssignment, MoveConstructor, MoveAssignment) will enforce explicit declaration of special member functions across the codebase, improving code clarity and preventing unintended behavior.Note: Line 11 correctly omits a trailing comma, following the repository's convention for list entries.
include/cpp_common/messages.hpp (1)
43-45: LGTM - Message class correctly made non-movable.Adding the defaulted destructor and deleted move operations completes the special member function set for this non-copyable, non-movable message accumulator class. This design is appropriate given the mutable
ostringstreammembers and the class's role as a message sink.include/cpp_common/assert.hpp (1)
148-152: LGTM - Exception class properly made copyable and movable.The defaulted copy and move constructors satisfy the requirement that exception classes be copy-constructible for throwing. Deleting the assignment operators is acceptable since exception objects are rarely reassigned after construction. This resolves the previous critical issue where deleted copy/move constructors would have prevented throwing the exception.
include/vrp/pd_problem.hpp (1)
48-51: LGTM! Explicit special member functions correctly implement the rule of five.The explicit declarations make the class semantics clear: the class is non-copyable and non-movable, which is appropriate given that the parameterized constructor sets static pointers across multiple classes. The defaulted destructor is correct since no special cleanup is required.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @.clang-tidy:
- Around line 14-24: The CheckOptions entries for
cppcoreguidelines-special-member-functions use invalid keys (DefaultConstructor,
CopyConstructor, CopyAssignment, MoveConstructor, MoveAssignment) which
clang-tidy will ignore; remove those invalid option keys and replace them only
with the documented options (AllowSoleDefaultDtor, AllowMissingMoveFunctions,
AllowMissingMoveFunctionsWhenCopyIsDeleted) as needed, ensuring the CheckOptions
list references the correct option names for
cppcoreguidelines-special-member-functions.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.clang-tidy
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-30T19:45:24.426Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:24.426Z
Learning: In .clang-tidy files (and similar list-based configuration files), the last entry in a list should not have a trailing comma, following project convention. Enforce that lists do not end with a comma to improve diffs and consistency across configuration files.
Applied to files:
.clang-tidy
📚 Learning: 2025-12-30T19:45:40.183Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:11-11
Timestamp: 2025-12-30T19:45:40.183Z
Learning: In any .clang-tidy configuration files, ensure that the last element of a list does not have a trailing comma, to follow the repository's coding convention. This applies to all pgRouting/.clang-tidy files across the codebase.
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)
10-12: LGTM! Trailing comma convention correctly followed.The additions to the Checks list correctly follow the project convention: the last entry (
cppcoreguidelines-special-member-functionson line 12) has no trailing comma, while preceding entries have trailing commas. Based on learnings, this improves diffs and consistency across configuration files.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @.clang-tidy:
- Around line 10-11: The .clang-tidy config mistakenly adds
cppcoreguidelines-prefer-member-initializer instead of the intended
cppcoreguidelines-special-member-functions; replace the incorrect check name
(cppcoreguidelines-prefer-member-initializer) with
cppcoreguidelines-special-member-functions in the checks list so the linter
enforces special-member-functions (copy/move/dtor) as the PR describes, and
verify there are no duplicate or conflicting entries for these check names
elsewhere in the config.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.clang-tidy
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-30T19:45:24.426Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:24.426Z
Learning: In .clang-tidy files (and similar list-based configuration files), the last entry in a list should not have a trailing comma, following project convention. Enforce that lists do not end with a comma to improve diffs and consistency across configuration files.
Applied to files:
.clang-tidy
📚 Learning: 2025-12-30T19:45:40.183Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:11-11
Timestamp: 2025-12-30T19:45:40.183Z
Learning: In any .clang-tidy configuration files, ensure that the last element of a list does not have a trailing comma, to follow the repository's coding convention. This applies to all pgRouting/.clang-tidy files across the codebase.
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @.clang-tidy:
- Around line 14-24: The CheckOptions block for
cppcoreguidelines-special-member-functions contains invalid keys
(DefaultConstructor, CopyConstructor, CopyAssignment, MoveConstructor,
MoveAssignment) which clang-tidy will ignore; either delete this CheckOptions
section to use defaults or replace it with valid options (AllowSoleDefaultDtor,
AllowMissingMoveFunctions, AllowMissingMoveFunctionsWhenCopyIsDeleted) and set
the desired values; update the .clang-tidy entry for
cppcoreguidelines-special-member-functions accordingly and remove the duplicate
comment block if present.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.clang-tidy
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-30T19:45:24.426Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:9-11
Timestamp: 2025-12-30T19:45:24.426Z
Learning: In .clang-tidy files (and similar list-based configuration files), the last entry in a list should not have a trailing comma, following project convention. Enforce that lists do not end with a comma to improve diffs and consistency across configuration files.
Applied to files:
.clang-tidy
📚 Learning: 2025-12-30T19:45:40.183Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 3004
File: .clang-tidy:11-11
Timestamp: 2025-12-30T19:45:40.183Z
Learning: In any .clang-tidy configuration files, ensure that the last element of a list does not have a trailing comma, to follow the repository's coding convention. This applies to all pgRouting/.clang-tidy files across the codebase.
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)
10-12: LGTM on the Checks list.The
cppcoreguidelines-special-member-functionscheck is now correctly added, and the list follows the convention of no trailing comma on the last entry.
|
Hi @cvvergara, I’ve resolved the .clang-tidy conflicts, fixed all alignment/linter issues, and all checks are passing. Could you please take a look and approve if everything looks good? |
…nimal header fixes
…-member-functions
…al-member-functions
2 participants
This PR adds the
cppcoreguidelines-special-member-functionscheck to the.clang-tidyconfiguration.Purpose of this check
Changes made:
.clang-tidyto include thecppcoreguidelines-special-member-functionscheckSummary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.