"Rule of 5" copy/assign/move declarations #601
Conversation
cary-ilm
commented
Oct 25, 2019
•
cary-ilm
commented
- Added =delete constructor/operator=/move to all internal classes (declared inside a .cpp)
- Added =delete constructor/operator=/move to all external classes (in .h files) for which the operation is not trivial (i.e. class has dynamically-allocated fields), and is currently not needed.
- Added =default constructor/operator=/move to TypedAttribute class, since that behavior is reasonable.
- added if(this!=&other) in operator=
- restructured IMF_STD_ATTRIBUTE
Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
- Added =delete copy constructors and assignment operators - Added =default destructors - check for &other==this in assignment operator Signed-off-by: Cary Phillips <cary@ilm.com>
- copy constructors and assignment operators declared = delete - use of ## is macro definition Signed-off-by: Cary Phillips <cary@ilm.com>
- copy constructor and assignment operator declared = delete - compare this==&other in assignment operator Signed-off-by: Cary Phillips <cary@ilm.com>
- removed const from operator= =delete - added move-assign/move-constructor = delete Signed-off-by: Cary Phillips <cary@ilm.com>
|
You probably saw that this is generating an undefined move constructor in the Windows build when linking testMultiPartSharedAttributes.obj. I suspect this is related to MSVC compiler generating a move constructor while gcc/clang are doing some optimization akin to copy elision. Possibly related to line 146 in testMultiPartSharedAttributes.cpp where header is an xvalue in the call to headers.push_back(header) but I haven't traced it to where the TypedAttr move constructor would actually be invoked. Just a guess as I haven't had time to test it! 2019-10-25T22:00:12.5717991Z testMultiPartSharedAttributes.obj : error LNK2019: unresolved external symbol "public: __thiscall Imf_2_4::TypedAttribute::TypedAttribute(class Imf_2_4::TypedAttribute const &&)" |
|
I guess in this case there's no reason to not provide the move constructor as well. These changes are taking the conservative approach - if the function isn't declared, eliminated it, as long as there's no legitimate need for it, but this presumably points to a need for it. It's worth investigating a little more, but I'll just add the move constructor. |
|
These copy/move deletions break ABI compatibility. And possibly API compatibility when both are deleted. Just to verify, we are ok with that? Any lib user who has been invoking these copy/move constructors intentionally or unintentionally, will not be able to compile, Perhaps not a bad thing, they can add the constructor if there is legitimate need? |
|
They are all things that shouldn't be copied or moved, so I'm ok with it. But the potential break break in ABI compatibility means it should not be backported to 2.4.x for sure (I'm ok with ABI breaks for minor releases such as 2.4 -> 2.5; IIRC, semver only talks about API, not ABI, so I think it's not a break of semver rules for 2.5). The potential break in API compatibility means it's probably a 3.0 feature if you want to follow strict sementic versioning. |
TypedAttribute assumes that the T class supports copy/move/assign operations, so it should, too. Also, the move/moveassign operators for OStream/IStream incorrectly declared their arguments as const. Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm
changed the title
|
Changed the name of the PR and edited the description, since the majority of the fixes involve copy/move/assign operations. |
Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
When we implemented the "rule of 5" (PR AcademySoftwareFoundation#601) in 2019, I added code to testAttributes.cpp to validate the behavior, specifically related to the TypedAttribute class, but in retrospect, this code isn't reliable, so I propose eliminating the test altogether. The test counts invocations of the various constructors, destructors, and move/assignment operators, expecting them to be consistent, but compilers can optionally optimize some of these operations away, producing inconsistent results. In particular, a Debug build on Windows produces different results from a Release build. Our CI has not historically tested a Windows Debug build, which explains why we never caught this. The exact behavior may have also changed with C++17. All the more reason to avoid such picky tests. This does not change the library code, only the tests. Signed-off-by: Cary Phillips <cary@ilm.com>
When we implemented the "rule of 5" (PR #601) in 2019, I added code to testAttributes.cpp to validate the behavior, specifically related to the TypedAttribute class, but in retrospect, this code isn't reliable, so I propose eliminating the test altogether. The test counts invocations of the various constructors, destructors, and move/assignment operators, expecting them to be consistent, but compilers can optionally optimize some of these operations away, producing inconsistent results. In particular, a Debug build on Windows produces different results from a Release build. Our CI has not historically tested a Windows Debug build, which explains why we never caught this. The exact behavior may have also changed with C++17. All the more reason to avoid such picky tests. This does not change the library code, only the tests. Signed-off-by: Cary Phillips <cary@ilm.com>
