clang-tidy: check and fix cppcoreguidelines-special-member-functions#3053
clang-tidy: check and fix cppcoreguidelines-special-member-functions#3053Mohit242-bit wants to merge 1 commit intopgRouting:developfrom
Conversation
WalkthroughMultiple headers were edited to explicitly declare special member functions: several classes made non-copyable/non-movable, some gained defaulted special members, and raw pointer ownership was replaced with 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/cpp_common/line_vertex.hpp (1)
58-63: 🧹 Nitpick | 🔵 TrivialConsider simplifying the copy constructor.
The user-defined copy constructor performs the same member-by-member copy that the compiler-generated default would provide. Consider defaulting it for consistency with the newly added defaulted assignment operator:
Line_vertex(const Line_vertex&) = default;This would also make the
cp_membersmethod potentially redundant.include/chinese/chinesePostman.hpp (1)
107-109: 🧹 Nitpick | 🔵 TrivialDestructor only clears one container; others auto-destruct.
The destructor explicitly clears
edgeToIdxwhile other member containers (originalEdges,resultEdges,resultGraph, etc.) are left to their automatic destruction. If explicit clearing is intended for a specific reason (e.g., deterministic order), consider documenting it. Otherwise, this manual clear is unnecessary since the map's destructor handles cleanup.Consider simplifying the destructor
PgrDirectedChPPGraph::~PgrDirectedChPPGraph() { - edgeToIdx.clear(); }Or if the destructor body becomes empty, you could default it in the declaration and remove the definition entirely.
🤖 Fix all issues with AI agents
In `@include/yen/ksp.hpp`:
- Line 66: Replace raw pointer ownership of m_vis with std::unique_ptr to avoid
manual new/delete: change the member declaration type of m_vis to
std::unique_ptr<Visitor>, initialize it with std::make_unique<Visitor>() (e.g.,
in the constructor initializer where currently m_vis(new Visitor) is used),
remove any manual delete or custom destructor and default the destructor, and
ensure all existing usages of m_vis-> remain unchanged; also apply the same
replacement for the other m_vis occurrence referenced in the diff.
There was a problem hiding this comment.
Pull request overview
This PR enables the cppcoreguidelines-special-member-functions clang-tidy check and fixes violations across 6 classes by adding missing special member functions according to the Rule of Five/Rule of Zero principle.
Changes:
- Removed
cppcoreguidelines-special-member-functionsfrom the disabled checks list in .clang-tidy - Added missing special member functions (copy constructors, copy assignment operators, destructors) to 6 classes across 5 header files
- Ensured non-copyable classes have deleted copy operations and copyable classes have appropriate implementations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| .clang-tidy | Removed cppcoreguidelines-special-member-functions from the disabled checks list |
| include/yen/ksp.hpp | Added deleted copy constructor and copy assignment operator to Pgr_ksp class; added default constructor and deleted copy operations to Visitor class |
| include/withPoints/withPoints.hpp | Added deleted copy assignment operator and default destructor to Pg_points_graph class |
| include/vrp/solution.hpp | Added default destructor to Solution class |
| include/cpp_common/line_vertex.hpp | Added default copy assignment operator and default destructor to Line_vertex class |
| include/chinese/chinesePostman.hpp | Added deleted copy constructor and copy assignment operator to PgrDirectedChPPGraph class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Mohit242-bit
force-pushed
the
fix/cppcoreguidelines-special-member-functions
branch
5 times, most recently
from
28ec4d6 to
7555f35
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/vrp/solution.hpp (1)
68-83:⚠️ Potential issue | 🟠 MajorRule of Five incomplete — move constructor and move assignment operator are missing.
The class declares a copy constructor, copy assignment operator, and destructor, but no move constructor or move assignment operator. Under the strict Rule of Five (which
cppcoreguidelines-special-member-functionsenforces by default), all five should be declared. The compiler will implicitly delete the move operations here since a copy constructor is user-declared, but the clang-tidy check may still flag this unlessAllowMissingMoveFunctionsis enabled.Other classes in this PR (e.g.,
Pg_points_graph,PgrDirectedChPPGraph) explicitly declare all five. Consider adding:Proposed fix
/* `@brief` copy assignment */ Solution& operator = (const Solution& sol) { EPSILON = 0.0001; fleet = sol.fleet; trucks = sol.trucks; return *this; } + /* `@brief` move constructor */ + Solution(Solution&&) = default; + + /* `@brief` move assignment */ + Solution& operator=(Solution&&) = default; + /* `@brief` destructor */ ~Solution() = default;#!/bin/bash # Check if AllowMissingMoveFunctions is configured in .clang-tidy fd '.clang-tidy' --max-depth 2 --exec cat {}include/cpp_common/line_vertex.hpp (1)
58-68: 🧹 Nitpick | 🔵 TrivialConsider defaulting the copy constructor as well.
The user-defined copy constructor (lines 58–63) manually copies every member, which is identical to what
= defaultwould generate. Since the new copy assignment, move operations, and destructor are all defaulted, the copy constructor could follow suit for consistency and reduced maintenance burden.Proposed simplification
- 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(const Line_vertex&) = default;
|
Hi @cvvergara , This PR addresses the remaining Summary of Changes:
Verified with local build . Would appreciate a review when you get a chance. |
- Modernize Pgr_ksp to use std::unique_ptr instead of raw pointer - Complete Rule of Five for Pgr_ksp, Visitor, Pg_points_graph, PgrDirectedChPPGraph, Solution, and Line_vertex classes - Update turnRestrictedPath to use std::make_unique
Mohit242-bit
force-pushed
the
fix/cppcoreguidelines-special-member-functions
branch
from
7555f35 to
6b7268d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/vrp/solution.hpp (2)
70-83:⚠️ Potential issue | 🟠 MajorRule of Five is incomplete — move constructor and move assignment are missing.
The class now declares a copy constructor, copy assignment operator, and destructor, but no move constructor or move assignment operator. The
cppcoreguidelines-special-member-functionscheck requires all five to be declared when any are user-provided. This will likely still trigger the clang-tidy warning this PR aims to fix.Either default or delete them:
Proposed fix
/* `@brief` copy assignment */ Solution& operator = (const Solution& sol) { EPSILON = 0.0001; fleet = sol.fleet; trucks = sol.trucks; return *this; } + /* `@brief` move constructor */ + Solution(Solution&&) = default; + + /* `@brief` move assignment */ + Solution& operator=(Solution&&) = default; + /* `@brief` destructor */ ~Solution() = default;
68-69: 🧹 Nitpick | 🔵 TrivialCopy constructor and copy assignment hardcode
EPSILONinstead of copying from source.Both the copy constructor (line 69:
EPSILON(0.0001)) and the copy assignment (line 76:EPSILON = 0.0001;) ignoresol.EPSILON. IfEPSILONis ever changed on an instance, the copy will silently reset it. This is pre-existing behavior, but worth flagging since the area is being touched.Proposed fix
Solution(const Solution &sol) : - EPSILON(0.0001), + EPSILON(sol.EPSILON), fleet(sol.fleet), trucks(sol.trucks) {}; Solution& operator = (const Solution& sol) { - EPSILON = 0.0001; + EPSILON = sol.EPSILON; fleet = sol.fleet; trucks = sol.trucks; return *this; }
1 participant
Fixes # 3042
#summary
This PR enables and fixes the
cppcoreguidelines-special-member-functionsclang-tidy check.Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
Refactor
Chores