Options: Delete copy constructor and copy assignment #2861
Conversation
Accidentally copying Options leads to unexpected bugs where changes (e.g documentation) changes a copy rather than the original. Usually what is intended is a reference, but the author omits an `&`. Copy assignment is also ambiguous: Is it the value, the attributes, or the children that should be copied? Now if a copy is really intended, there is a `.copy()` method (thanks Rust!)
|
clang-tidy review says "All clean, LGTM! 👍" |
Should be a reference. Caught because copy constructor is deleted.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
C++ initializer_lists don't play nicely with uncopyable types https://blog.knatten.org/2018/10/05/why-you-cant-list-initialize-containers-of-non-copyable-types/ List initialization of Options now constructs nested initializer lists of `Options::CopyableOptions`, then walks this tree to move data into Options. This simplifies some of the logic, since we're now traversing the tree of Options starting from the root.
Tests that use copy constructor changed to test copy method. Unit tests compile but failing. Probably initializer_list construction issue.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
When setting a value, the `is_section` member should be `false`, and the "source" attribute must be set.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Looks like the failing tests are just because a fedora mirror 404'd, rerunning now. LGTM, thanks @bendudson! In C++26 we might even be able to do Please could you put a line under "breaking changes" in |
|
For some reason, only the Fedora build is failing with: Not sure why the other compilers aren't catching that! Will investigate locally |
|
Oh, this is because most of our builds use sundials 4.1.0, and we have a guard BOUT-dev/src/solver/impls/cvode/cvode.cxx Lines 350 to 358 in 283917d which is not correct. I'll make a separate PR for that fix (we also have some deprecations to fix, I'll have a stab at those too) |
In `create_constraints` the variable options were being copied, so that the `positivity_constraint` documentation was in a copy that was discarded. Should use a reference rather than copy.
When a compiler error is issued, it will usually print the line where the method is deleted.
|
clang-tidy review says "All clean, LGTM! 👍" |
C++ compilers can cast char* to bool, rather than std::string.
|
clang-tidy review says "All clean, LGTM! 👍" |
Accidentally copying Options leads to unexpected bugs where changes (e.g documentation) changes a copy rather than the original. Usually what is intended is a reference, but the author omits an
&.Example: bendudson/hermes-3#204 (comment)
Copy assignment is also ambiguous: Is it the value, the attributes, or the children that should be copied?
Now if a copy is really intended, there is a
.copy()method (thanks Rust!) that makes a deep copy.