Skip to content

Conversation

@vkucera
Copy link
Collaborator

@vkucera vkucera commented Dec 9, 2025

  • Define missing special member functions from the rule of five.
  • Declare move functions noexcept.
  • Move assignment operator for TableIterator not defined as it cannot be defaulted.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2025

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0
async-2024-PbPb-apass1
async-2024-ppRef-apass1
async-2024-PbPb-apass2
async-2023-PbPb-apass5

return this->desc_hash == _desc_hash;
}

constexpr ~TableRef() = default;
Copy link
Member

Choose a reason for hiding this comment

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

This is a trivial struct that does not require a destructor. Since it contains three integers, for which move and copy identical, there is no need to declare everything else as well.

}

ColumnIterator() = default;
~ColumnIterator() = default;
Copy link
Member

Choose a reason for hiding this comment

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

Again, the destructor only makes sense for the things that own dynamically allocated memory directly, especially if there is a polymorphic hierarchy. This class only contains observer pointers, so there is really no need for the move constructors either.

@ktf
Copy link
Member

ktf commented Dec 10, 2025

I am not sure this is a good idea. Have you checked with bloaty this change does not add to the already large number of symbols we have? Is this trying to solve an actual problem or you simply want to have the linter happy?

@vkucera
Copy link
Collaborator Author

vkucera commented Dec 10, 2025

I am not sure this is a good idea. Have you checked with bloaty this change does not add to the already large number of symbols we have?

I haven't but I'm happy to check if you point me to instructions.

Is this trying to solve an actual problem or you simply want to have the linter happy?

Both.

  • Implicitly deleted move constructors and move assignments prevent optimisations.
  • When I run Clang-Tidy on O2Physics with the cppcoreguidelines-special-member-functions check, I get thousands of errors from ASoA for all columns, which makes it harder to find errors related to classes in O2Physics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants