Re-enable C++20 aggregate initialization for CSerializedNetMsg #24641

maflcko · 2022-03-22T15:35:51Z

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 213e98c. This pull request is inspired by section 3.3 of the paper http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf

ryanofsky · 2022-03-22T18:35:52Z

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:

When you have a class that is expensive to copy, and a cheap to move, and you want to disable default copy behavior to avoid slow copying by accident.
When you are using a low level C API, and are trying to wrap up low level C pointers or file descriptors or handles in RAII classes.

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

maflcko · 2022-03-23T08:35:14Z

Thx, edited OP/title and reworked code.

ryanofsky

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

ryanofsky · 2022-03-23T15:59:58Z

src/util/types.h

In commit "Add DISABLE_IMPLICIT_COPIES helper" (fa540a3c5a08c06917a3df1b3d42939eab757f5e)

Can drop default constructor? Seems orthogonal to copy/move stuff

ryanofsky · 2022-03-23T16:12:32Z

src/util/types.h

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.

warning: explicitly defaulted copy assignment operator is implicitly deleted [-Wdefaulted-function-deleted] error: 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 deleted function can not be "recovered" by defaulting.

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/CopyFrom macro 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.

maflcko · 2022-03-24T14:17:31Z

I've reverted this pull to the initial version

sipa · 2022-03-24T14:31:32Z

src/net.h

    std::string m_type;
+
+    // No implicit copying, only moves.
+    DISABLE_IMPLICIT_COPIES();


Adding a field to a struct/class will always increase its size, even if the added type is empty (because no object can have size 0). As an alternative, does inheriting from Disable_Implicit_Copies work (you can inherit from an empty struct without increasing size).

According to the language in p1008 "An aggregate is an array or a class with no user-provided, explicit,user-declared or inherited constructors" this would make the class no longer compatible with aggregate initialization in c++20

The paper recommended to use no_unique_address for the field. Though, that is C++20 only: https://en.cppreference.com/w/cpp/language/attributes/no_unique_address

maflcko · 2022-03-29T09:53:58Z

Closing, as it seems impossible without downsides

maflcko added the Refactoring label Mar 22, 2022

maflcko mentioned this pull request Mar 22, 2022

build: Add --enable-c++20 option #24169

Merged

maflcko changed the title ~~Add AGGREGATE_NO_COPY_ONLY_MOVE helper~~ Add DISABLE_IMPLICIT_COPIES helper Mar 23, 2022

maflcko force-pushed the 2203-copy-move- branch from fa74f6c to fa540a3 Compare March 23, 2022 08:26

ryanofsky approved these changes Mar 23, 2022

View reviewed changes

maflcko marked this pull request as draft March 24, 2022 08:57

DrahtBot added the Needs rebase label Mar 24, 2022

maflcko changed the title ~~Add DISABLE_IMPLICIT_COPIES helper~~ Re-enable C++20 aggregate initialization for CSerializedNetMsg Mar 24, 2022

maflcko marked this pull request as ready for review March 24, 2022 14:13

maflcko force-pushed the 2203-copy-move- branch from fa540a3 to fa57a80 Compare March 24, 2022 14:13

Add DISABLE_IMPLICIT_COPIES helper

fada2c9

maflcko force-pushed the 2203-copy-move- branch from fa57a80 to fada2c9 Compare March 24, 2022 14:16

sipa reviewed Mar 24, 2022

View reviewed changes

DrahtBot removed the Needs rebase label Mar 24, 2022

maflcko closed this Mar 29, 2022

maflcko deleted the 2203-copy-move-🌺 branch March 29, 2022 09:54

bitcoin locked and limited conversation to collaborators Mar 29, 2023

Re-enable C++20 aggregate initialization for CSerializedNetMsg #24641

Re-enable C++20 aggregate initialization for CSerializedNetMsg #24641

Uh oh!

Conversation

maflcko commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 22, 2022

Uh oh!

maflcko commented Mar 23, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 24, 2022

Uh oh!

sipa Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maflcko commented Mar 22, 2022 •

edited

Loading

sipa Mar 24, 2022 •

edited

Loading

ryanofsky Mar 24, 2022 •

edited

Loading