Skip to content

Conversation

@avdg81
Copy link
Contributor

@avdg81 avdg81 commented Nov 5, 2025

📝 Description
The aim of this PR is to make class Flags moveable. So far, it wasn't, since it explicitly defined the destructor and the copy operations (i.e. the copy constructor and the copy assignment operator). As a result, the move operations are not automatically supplied by the compiler (and thus we miss out on an optimization opportunity, since "moves" will become copies). For those who need a refresher on this topic, Howard Hinnant gives a very clear explanation in one of his presentations. In particular, starting from 13:21 he explains under which circumstances special members are provided by the compiler (and when they are not).

Since Flags is the base class of many other classes, those classes in turn are non-moveable, too. However, we don't see a fundamental objection against making Flags moveable. In fact, when it becomes moveable, other classes can be made moveable as well. To limit the scope of this PR as much as possible, we haven't touched any other classes. We may want to do that in follow-up PRs.

🆕 Changelog

  • Use = default for special member functions (i.e. the default constructor, the (virtual) destructor, the copy constructor, and the copy assignment operator) rather than writing down the code that the compiler can generate for us. Note that we are using the "Rule of Five" here, since we need a virtual destructor. And, as Howard Hinnant explains, when a destructor is user-declared, the move operations (i.e. the move constructor and the move assignment operator) won't be declared. That's why we explicitly declare them here (including our request to the compiler to provide default implementations for them).
  • The data members mIsDefined and mFlags are initialized in-class, and they are explicitly default-constructed. See this rule in the C++ Core Guidelines of why this is a good idea (the text below has been quoted from this rule):

Beware that built-in types are not properly default constructed:
(...)
Statically allocated objects of built-in types are by default initialized to 0, but local built-in variables are not. (...)
Assuming that you want initialization, an explicit default initialization can help:
(...)

Also use `= default` for special member functions (i.e. the default constructor, the (virtual) destructor, the copy constructor, and the copy assignment operator) rather than writing down the code that the compiler can generate for us.

The class's data member (i.e. `mIsDefined` and `mFlags`) are best initialized in-class.
@avdg81 avdg81 self-assigned this Nov 5, 2025
@avdg81 avdg81 added Kratos Core C++ Behaviour Change changes behaviour but not the API labels Nov 5, 2025
@avdg81 avdg81 moved this to 👷 In Progress in Kratos Product Backlog Nov 5, 2025
@avdg81
Copy link
Contributor Author

avdg81 commented Nov 10, 2025

Just a friendly reminder to @roigcarlo and @pooyan-dadvand that we would like to have your opinion on this PR. Thank you.

@avdg81
Copy link
Contributor Author

avdg81 commented Nov 17, 2025

Just another friendly reminder to @roigcarlo and @pooyan-dadvand that we would like to have your opinion on this PR. Thank you.

@avdg81 avdg81 moved this from 👷 In Progress to 👀 In Review in Kratos Product Backlog Nov 18, 2025
@avdg81 avdg81 moved this to 👀 In Review in Kratos Product Backlog Dec 2, 2025
Copy link
Member

@roigcarlo roigcarlo left a comment

Choose a reason for hiding this comment

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

Hey we have taken a look with @pooyan-dadvand and we have nothing against it. 👍

@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in Kratos Product Backlog Dec 3, 2025
@avdg81 avdg81 marked this pull request as ready for review December 4, 2025 08:53
@avdg81 avdg81 requested a review from a team as a code owner December 4, 2025 08:53
@avdg81 avdg81 merged commit ac2a8f4 into master Dec 4, 2025
10 checks passed
@avdg81 avdg81 deleted the geo/experimental-addition-of-move-operations branch December 4, 2025 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Behaviour Change changes behaviour but not the API C++ Kratos Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants