Apply Rule of Zero to utility/keys container family (corrected)#705
Open
lyskov-ai wants to merge 1 commit into
Open
Apply Rule of Zero to utility/keys container family (corrected)#705lyskov-ai wants to merge 1 commit into
lyskov-ai wants to merge 1 commit into
Conversation
Applies Rule of Zero to five sibling key-container templates in `source/src/utility/keys/`: - `ClassKeyMap` - `ClassKeyVector` - `KeyVector` - `SmallKeyMap` - `SmallKeyVector` Each held only standard-container value-type members (a `Vector`, plus an `IndexMap` and a scalar `Index u_` in the `Small*` variants). Their user-declared destructors (empty body or `= default`), copy constructors, and copy-assignment operators were byte-for-byte equivalent to the implicit defaults the compiler would synthesize, so they were redundant. Removing them lets the implicitly defaulted special-member-functions take over and, as a side effect, restores the implicit move constructor and move assignment that were previously suppressed by the user-declared copy operations. Explicit `= default` default constructors are kept on all five classes. `SmallKeyMap` / `SmallKeyVector` need a user-defined default constructor to value-initialize the scalar `Index u_`, which an implicit default would leave indeterminate. `ClassKeyMap` / `ClassKeyVector` / `KeyVector` need an explicit `= default` because each has a user-declared (non-default) constructor (iterator-range or size/value), and a class with any user-declared constructor does not get an implicitly synthesized default. The previous attempt at this refactor (RosettaCommons#691, reverted by RosettaCommons#703) dropped these `= default` defaults on the assumption that they would be synthesized implicitly, which broke `ClassKeyMap m;` in test/utility/keys/ ClassKeyMap.cxxtest.hh. Keeping the explicit defaults preserves the original API while still letting copy / move / destructor be implicit. Supersedes RosettaCommons#704.
lyskov
approved these changes
May 18, 2026
roccomoretti
approved these changes
May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-applies the Rule-of-Zero refactor that was originally landed as #691 and then reverted in #703, fixing the bug that caused the revert. Supersedes #704 (the revert-revert), which still ships the same broken state and will fail
cat=testontest/utility/keys/ClassKeyMap.cxxtest.hh.What was broken in #691 / #704
The original commit dropped the explicit
= defaultdefault constructors fromClassKeyMap,ClassKeyVector, andKeyVectoron the assumption that they would be synthesized implicitly. They are not. Each of those classes has a user-declared (non-default) constructor — the templated iterator-range constructor onClassKeyMap, and the size / uniform-value / iterator-range constructors onClassKeyVectorandKeyVector. By the C++ rules, any user-declared constructor suppresses the implicit default. That brokeMap m;in the unit test (test/utility/keys/ClassKeyMap.cxxtest.hh):What this PR does
Same Rule-of-Zero cleanup as #691 — drops the redundant user-declared destructors, copy constructors, and copy-assignment operators on all five classes, so the implicitly defaulted versions (now including move constructor and move assignment) take over — but keeps an explicit
= defaultdefault constructor onClassKeyMap,ClassKeyVector, andKeyVector.SmallKeyMapandSmallKeyVectoralready keep a user-defined default constructor because they need to value-initialize the scalarIndex u_.Verified by building
mode=debug cat=testend-to-end (no compile errors).