refactor: Improve special member functions definitions

hebasto commented at 2:32 pm on August 17, 2019: member

This PR fixes special member functions in the following class/struct definitions:

struct PartiallySignedTransaction (done in #17349) The default (i.e., generated by a compiler) copy constructor does the same things. It prevents -Wdeprecated-copy warning in GCC 9 (#15822) and -Wdeprecated warning in Clang 10 for implicitly declared operator=
class SigningProvider All special member functions of the SigningProvider class defined =default which makes it a well-behaved base class. It prevents -Wdeprecated warning in Clang 10
struct secure_allocator Define =default copy assignment explicitly. It prevents -Wdeprecated warning in Clang 10
struct FrozenCleanupCheck Define =default copy constructor explicitly. It prevents -Wdeprecated warning in Clang 10

DrahtBot added the label Refactoring on Aug 17, 2019

emilengler commented at 9:42 pm on August 17, 2019: contributor

Concept ACK 5b2bb0f See 385-386 in src/psbt.h and 12-13 in src/psbt.cpp

practicalswift commented at 6:32 am on August 18, 2019: contributor

@hebasto Is this change complete in the sense that it addresses all instances of redundant copy constructors in our code base? :-)

hebasto commented at 7:46 am on August 18, 2019: member

@practicalswift

Is this change complete in the sense that it addresses all instances of redundant copy constructors in our code base? :-)

I don’t know (( What tool could be helpful to check our codebase? It seems GCC 9 compiler do this work, right?

practicalswift commented at 11:09 am on August 18, 2019: contributor

@hebasto Isn’t it -Wdeprecated-copy in gcc and -Wdeprecated in clang? :-)

hebasto commented at 3:03 pm on August 18, 2019: member

@practicalswift on master:

0$ gcc --version | grep gcc
1gcc (GCC) 9.1.1 20190503 (Red Hat 9.1.1-1)
2
3$ make 2>&1 | grep -e "-Wdeprecated-copy" | grep -vE '(^/usr|leveldb)'
4psbt.cpp:335:19: warning: implicitly-declared ‘PartiallySignedTransaction& PartiallySignedTransaction::operator=(const PartiallySignedTransaction&)’ is deprecated [-Wdeprecated-copy]

promag commented at 11:51 pm on August 18, 2019: member

ACK 5b2bb0fd3af34463bb382b473cc8b6dee65c0920. Being complete is nice, disallowing more of these would be great.

fanquake commented at 8:19 am on August 21, 2019: member

I’m seeing more of these warnings when compiling using Apple Clang with -Wdeprecated:

0./psbt.h:398:5: warning: definition of implicit copy assignment operator for 'PartiallySignedTransaction' is deprecated because it has a user-declared copy constructor [-Wdeprecated]
1./script/signingprovider.h:21:13: warning: definition of implicit copy assignment operator for 'SigningProvider' is deprecated because it has a user-declared destructor [-Wdeprecated]
2./script/signingprovider.h:21:13: warning: definition of implicit copy constructor for 'SigningProvider' is deprecated because it has a user-declared destructor [-Wdeprecated]
3test/checkqueue_tests.cpp:115:5: warning: definition of implicit copy constructor for 'FrozenCleanupCheck' is deprecated because it has a user-declared destructor [-Wdeprecated]
4./support/allocators/secure.h:35:5: warning: definition of implicit copy assignment operator for 'secure_allocator<unsigned char>' is deprecated because it has a user-declared destructor [-Wdeprecated]

0clang -v
1Apple LLVM version 10.0.1 (clang-1001.0.46.4)
2Target: x86_64-apple-darwin18.7.0
3Thread model: posix
4InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

DrahtBot commented at 5:47 am on August 24, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

hebasto commented at 7:17 pm on September 3, 2019: member

@fanquake

I’m seeing more of these warnings when compiling using Apple Clang with -Wdeprecated…

With the latest push:

0$ clang --version | grep version
1Apple LLVM version 10.0.1 (clang-1001.0.46.4)
2$ ./configure CXXFLAGS="-Wdeprecated"
3...
4$ make 2>&1 | grep -e "\[-Wdeprecated\]" | grep -vE '^/usr|leveldb|univalue'

the output has no mentioned warnings.

hebasto renamed this:
~~refactor: Remove redundant PSBT copy constructor~~
refactor: Improve special member functions definitions
on Sep 3, 2019

hebasto commented at 7:57 pm on September 3, 2019: member

The PR description has been updated.

hebasto commented at 9:40 am on October 5, 2019: member

@practicalswift Would you mind review this PR?

refactor: Improve SigningProvider base class

All special member functions of the SigningProvider class defined 
=default which makes it a well-behaved base class.

2581abf9aa

refactor: Define =default copy assignment explicitly c526c1db61

refactor: Define =default copy ctor explicitly fb5dc19ec6

hebasto force-pushed on Nov 4, 2019

hebasto commented at 3:55 pm on November 4, 2019: member

Rebased on top of #17349.

in src/test/checkqueue_tests.cpp:114 in fb5dc19ec6

111@@ -112,6 +112,7 @@ struct FrozenCleanupCheck {
112         return true;
113     }
114     FrozenCleanupCheck() {}

MarcoFalke commented at 4:14 pm on November 4, 2019:

why is this needed?

hebasto commented at 5:04 pm on November 4, 2019:

The default ctor is required at least here: https://github.com/bitcoin/bitcoin/blob/94a26b192f187cb50bf1ac1775b23f2b03f772b1/src/checkqueue.h#L150

practicalswift commented at 5:21 pm on November 4, 2019:

Unrelated, but is there any reason we’re doing the extra move here (push_back instead of emplace_back)?

MarcoFalke commented at 5:40 pm on November 4, 2019:

The default ctor is required at least here:

Yeah, but how is the manually defined ctor different from the compiler generated?

hebasto commented at 5:50 pm on November 4, 2019:

https://en.cppreference.com/w/cpp/language/default_constructor

If no user-declared constructors of any kind are provided for a class type (struct, class, or union), the compiler will always declare a default constructor as an inline public member of its class.

But we have a copy ctor with this PR, so the compiler will not generate a default ctor.

MarcoFalke commented at 5:59 pm on November 4, 2019:

So why do you provide the copy ctor? How is it different from the compiler generated one?

MarcoFalke commented at 6:00 pm on November 4, 2019:

Does providing a dtor make the compiler generated ctors go away?

hebasto commented at 6:10 pm on November 4, 2019:

Does providing a dtor make the compiler generated ctors go away?

No, providing a destructor does not make the compiler generated constructors go away. But the user-defined copy constructor (with this PR) makes the compiler generated default constructor go away.

My GCC fails to compile this PR without the default constructor. It needs FrozenCleanupCheck() {} or FrozenCleanupCheck() = default;

MarcoFalke commented at 6:12 pm on November 4, 2019:

Idk. It passed for me after I removed all ctors

hebasto commented at 6:22 pm on November 4, 2019:

… I removed all ctors

That is the point ;)

Did Clang 10 fire any warning with passed -Wdeprecated ?

MarcoFalke commented at 6:27 pm on November 4, 2019:

I don’t have clang 10, and it seems to be in development. I don’t think we fix compiler warnings of compilers that are in alpha stage.

hebasto commented at 6:43 pm on November 4, 2019:

I don’t have clang 10, and it seems to be in development.

macOS 10.14 Mojave, XCode 10.0:

0$ clang --version | grep version
1Apple LLVM version 10.0.1 (clang-1001.0.46.4)

fanquake commented at 6:46 pm on November 4, 2019:

Apple LLVM version 10.0.1 (clang-1001.0.46.4)

This version of Apple Clang is likely based off of ~ upstream Clang 6.0.1.

hebasto commented at 6:56 pm on November 4, 2019:

Did Clang 10 fire any warning with passed -Wdeprecated ?

So, I mean a compiler on macOS 10.14+ ;)

MarcoFalke commented at 7:28 pm on November 4, 2019:

Well, thank god I don’t have macOS, which is horribly broken in every thinkable way

hebasto commented at 8:20 pm on November 4, 2019: member

@practicalswift Would you mind reviewing and testing this PR?

hebasto closed this on Aug 18, 2020

DrahtBot locked this on Feb 15, 2022

refactor: Improve special member functions definitions #16641

Conflicts