I think this is a nice macro that can be used to disable implicit copies, but allow moves.
This is also required in the context of C++20 to re-enable aggregate initialization, which have been disabled in commit 213e98ca826eb25c7d6e26729c6a3de6521614ba. This pull request is inspired by section 3.3 of the paper http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf
Concept ACK fa74f6c5390a433342ce04359f592483a8d8cf97, but I think it would be worth trying to improve the macro if it is going to spread.
I was initially confused by PR description, but it seems like problem this solves is that C++20 forbids aggregate initialization if a class has any constructors, while C++17 allows it and lets you have constructors and aggregate initialization together at the same time. Because we want to use aggregate initialization, we will be forced to get rid of some constructors when we update to c++20, and the macro helps work around that.
I’m not sure this macro is ideal, though, and I think it might be good to try to improve it. I’m mainly concerned it will be applied places it doesn’t belong. Two cases where no-copy-only-move behavior is useful are:
I don’t think we have a lot of examples of the second case, so can probably ignore it. But for the first case, the macro sucks a little because it disable copying entirely instead of disabling copying by default which makes code annoying to work with and test, especially since when you test you often do want to be able to copy entire data structures. Also starting the name with AGGREGATE_ probably means developers will apply macro to aggregate initialized structs freely, which will make this even more annoying.
I think a better version of this macro would be called something like DISABLE_IMPLICIT_COPIES and would disable the copy constructor and assignment methods, while adding T Copy() const and void CopyFrom(const T&) methods that do allow convenient copying where intended
15+ * Explicit Copy() or CopyFrom() can be used where slow copies are desired.
16+ *
17+ * To use, place this as the last line into a class or struct.
18+ */
19+#define DISABLE_IMPLICIT_COPIES(T) \
20+ T() = default; \
In commit “Add DISABLE_IMPLICIT_COPIES helper” (fa540a3c5a08c06917a3df1b3d42939eab757f5e)
Can drop default constructor? Seems orthogonal to copy/move stuff
29+ *this = from; \
30+ } \
31+ \
32+private: \
33+ T& operator=(const T&) = default; \
34+ T(const T&) = default
In commit “Add DISABLE_IMPLICIT_COPIES helper” (fa540a3c5a08c06917a3df1b3d42939eab757f5e)
I think after adding C++20 support might need to drop these T constructors and go back to using the NoCopyOnlyMove _no_copy_only_move empty member trick, because according to p1008 if a class has any user-declared constructors it will no longer be an aggregate.
This only applies to constructors, not operator= methods, though, so I think private operator= method would be able to stay, and CopyFrom method would not have to change. Copy method might need to change to T Copy() const { T copy; copy = *this; return copy; }.
But it might make sense keep this current implementation for now and change it if needed in the C++20 PR later.
It looks like an implicitly deleted function can not be “recovered” by defaulting.
0warning: explicitly defaulted copy assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
1error: object of type 'CSerializedNetMsg' cannot be assigned because its copy assignment operator is implicitly deleted
Thus, it seems impossible to have a Copy/CopyFrom macro and aggregate initialization that works with both C++17 and C++20.
It looks like an implicitly
deletedfunction can not be “recovered” bydefaulting.
I don’t think the followup I’m suggesting should cause the copy asssignment operator to be implicitly deleted, so there be no need to recover it. But the PR in its current form seems fine, and we can iterate on it. If iteraton requires dropping explicit Copy methods to support c++20, that seems sad but not tragic.
Thus, it seems impossible to have a
Copy/CopyFrommacro and aggregate initialization that works with both C++17 and C++20.
I’m not convinced of this, but there’s no need to solve it now, and I think the macro here already provides an improvement. It would be good to have a unit test to check that the macro allows all the things it is supposed to allow in c++17. If it has to drop support for some of those things in c++20, the test could catch that and be updated at that time to reflect this.
I’m not convinced of this
Heh, what about you open a pull to prove me wrong?
I’m not convinced of this
Heh, what about you open a pull to prove me wrong?
I can try later, but I like this PR because it adds a simple and clean macro for c++17. If the macro has to get messier or stupider later to support c++20, I think it’s better if that happens in a followup PR adding c++20 support than in an earlier PR before we are even allowed to use c++20 features.
My thinking is that it is possible to achieve without a macro, manually implementing Copy/CopyFrom as seen here: https://github.com/bitcoin/bitcoin/pull/24169/files#diff-422879cc8bfac56d4380c865f381b58afeb344bc355bbc7f47c581e4491b6b4b
I will rebase this pull after #24169 and probably drop the Copy/CopyFrom from the macro. If there is a way to achieve them with a macro, it can be done later.
Thanks! Code review ACK fa540a3c5a08c06917a3df1b3d42939eab757f5e
It might be good to add a simple unit test to make sure aggregate initialization works, and check that https://en.cppreference.com/w/cpp/types/is_copy_constructible is false
109@@ -117,7 +110,11 @@ struct CSerializedNetMsg {
110
111 std::vector<unsigned char> data;
112 std::string m_type;
113+
114+ // No implicit copying, only moves.
115+ DISABLE_IMPLICIT_COPIES();
no_unique_address for the field. Though, that is C++20 only: https://en.cppreference.com/w/cpp/language/attributes/no_unique_address
Labels
Refactoring