Skip to content

[FastPR][Core] Missing move constructor and assignment in sparse graphs#13650

Merged
rubenzorrilla merged 7 commits intomasterfrom
core/sparse-graphs-move-ctors
Jul 22, 2025
Merged

[FastPR][Core] Missing move constructor and assignment in sparse graphs#13650
rubenzorrilla merged 7 commits intomasterfrom
core/sparse-graphs-move-ctors

Conversation

@rubenzorrilla
Copy link
Member

📝 Description
Adding the missing move constructor and move assignment operator to the SparseContiguousRowGraph and SparseGraph classes.

@rubenzorrilla rubenzorrilla self-assigned this Jul 18, 2025
@rubenzorrilla rubenzorrilla requested a review from a team as a code owner July 18, 2025 15:10
@rubenzorrilla rubenzorrilla added Incomplete FastPR This Pr is simple and / or has been already tested and the revision should be fast labels Jul 18, 2025
loumalouomega
loumalouomega previously approved these changes Jul 18, 2025
Co-authored-by: Máté Kelemen <44344022+matekelemen@users.noreply.github.com>
rubenzorrilla and others added 5 commits July 21, 2025 11:26
Co-authored-by: Máté Kelemen <44344022+matekelemen@users.noreply.github.com>
Co-authored-by: Máté Kelemen <44344022+matekelemen@users.noreply.github.com>
Co-authored-by: Máté Kelemen <44344022+matekelemen@users.noreply.github.com>
Copy link
Member

@RiccardoRossi RiccardoRossi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on my side i would approve, but i let @matekelemen doint it.

Copy link
Contributor

@matekelemen matekelemen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx 👌

Just one comment for future reference; I wouldn't want to re-trigger the CI for something so trivial:

/// Assignment operator
SparseGraph& operator=(SparseGraph const& rOther)=delete;

I want to make it clear to everyone writing annotations: unless they're doing something non-standard, there is no point in documenting built-in functions.

Why? Exactly because they're doing something standard. Anyone looking at the signature should already know what that function does, and how it's supposed to behave.

If you start annotating these standard functions, you'll have to explicitly define every auto-generated constructor / assignment operator / destructor and annotate them the same way you do here to be consistent. That'd lead to an unbelievably bloated code base and documentation. They're defined by the standard and auto-generated by the compiler for a reason.

@rubenzorrilla
Copy link
Member Author

thx 👌

Just one comment for future reference; I wouldn't want to re-trigger the CI for something so trivial:

/// Assignment operator
SparseGraph& operator=(SparseGraph const& rOther)=delete;

I want to make it clear to everyone writing annotations: unless they're doing something non-standard, there is no point in documenting built-in functions.

Why? Exactly because they're doing something standard. Anyone looking at the signature should already know what that function does, and how it's supposed to behave.

If you start annotating these standard functions, you'll have to explicitly define every auto-generated constructor / assignment operator / destructor and annotate them the same way you do here to be consistent. That'd lead to an unbelievably bloated code base and documentation. They're defined by the standard and auto-generated by the compiler for a reason.

I totally agree. It's pretty annoying documenting this just to keep consistency with the rest of files.

@rubenzorrilla rubenzorrilla merged commit 693971c into master Jul 22, 2025
10 checks passed
@rubenzorrilla rubenzorrilla deleted the core/sparse-graphs-move-ctors branch July 22, 2025 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FastPR This Pr is simple and / or has been already tested and the revision should be fast Incomplete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants