review ACK 25f19ef

cr ACK 25f19ef: patch looks correct

Note to reviewers: -Wdeprecated-copy is enabled as part of g++ -Wextra

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• build: Add -Werror=implicit-fallthrough compile flag #21430 (build: Add -Werror=implicit-fallthrough compile flag by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Code Review ACK 14cb2e0fe1384f89b4c9ca70751e6676f438b7a5

because the initial issue is no longer a thing.

? The issue is definitely still a thing. If you compile this PR on Ubuntu 18.04 with the system packages (Qt 5.9.5) and Clang 10, you're still going to see 100s of warnings, because nothing has changed there since #18738. i.e:

/usr/include/x86_64-linux-gnu/qt5/QtWidgets/qstyleoption.h:620:106: note: in implicit copy assignment operator for 'QStyleOptionTitleBar' first required here
    QStyleOptionTitleBar(const QStyleOptionTitleBar &other) : QStyleOptionComplex(Version, Type) { *this = other; }
                                                                                                         ^
/usr/include/x86_64-linux-gnu/qt5/QtWidgets/qstyleoption.h:640:5: warning: definition of implicit copy assignment operator for 'QStyleOptionGroupBox' is deprecated because it has a user-declared copy constructor [-Wdeprecated-copy]
    QStyleOptionGroupBox(const QStyleOptionGroupBox &other) : QStyleOptionComplex(Version, Type) { *this = other; }
    ^
/usr/include/x86_64-linux-gnu/qt5/QtWidgets/qstyleoption.h:640:106: note: in implicit copy assignment operator for 'QStyleOptionGroupBox' first required here
    QStyleOptionGroupBox(const QStyleOptionGroupBox &other) : QStyleOptionComplex(Version, Type) { *this = other; }
                                                                                                         ^
/usr/include/x86_64-linux-gnu/qt5/QtWidgets/qstyleoption.h:654:5: warning: definition of implicit copy assignment operator for 'QStyleOptionSizeGrip' is deprecated because it has a user-declared copy constructor [-Wdeprecated-copy]
    QStyleOptionSizeGrip(const QStyleOptionSizeGrip &other) : QStyleOptionComplex(Version, Type) { *this = other; }
    ^
/usr/include/x86_64-linux-gnu/qt5/QtWidgets/qstyleoption.h:654:106: note: in implicit copy assignment operator for 'QStyleOptionSizeGrip' first required here
    QStyleOptionSizeGrip(const QStyleOptionSizeGrip &other) : QStyleOptionComplex(Version, Type) { *this = other; }
                                                                                                         ^
/usr/include/x86_64-linux-gnu/qt5/QtWidgets/qstyleoption.h:670:5: warning: definition of implicit copy assignment operator for 'QStyleOptionGraphicsItem' is deprecated because it has a user-declared copy constructor [-Wdeprecated-copy]
    QStyleOptionGraphicsItem(const QStyleOptionGraphicsItem &other) : QStyleOption(Version, Type) { *this = other; }
    ^
/usr/include/x86_64-linux-gnu/qt5/QtWidgets/qstyleoption.h:670:107: note: in implicit copy assignment operator for 'QStyleOptionGraphicsItem' first required here
    QStyleOptionGraphicsItem(const QStyleOptionGraphicsItem &other) : QStyleOption(Version, Type) { *this = other; }
                                                                                                          ^
1 warning generated.
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin_locale.o
24 warnings generated.
  AR       libbitcoin_server.a
24 warnings generated.

Tested on the following systems:

It's not really surprising that after testing on systems that have either a very recent release, or updated LTS version of Qt, you didn't see any warnings, as they either have the patch that fixed the warnings, or it's been backported.

If you compile this PR on Ubuntu 18.04 with the system packages (Qt 5.9.5) and Clang 10 ...

Is this even a commonly supported config? I'd presume that GUI users are faster to update to the latest release or LTS or maybe even use the snap/flatpak/static bins.

Is this even a commonly supported config?

I wouldn't think it'd be that common, and you should be able to assume that anyone compiling their own binaries will understand the warnings, and is capable of passing -Wno-deprecated-copy if they don't want to see them.

My point here is that claiming a problem is "no longer a thing", and then only testing for it in places where it's obviously not going to occur, doesn't make heaps of sense.

I'm still ok with merging this, and will do shortly. If anything, it might serve as a barometer as to whether there are a number of users using that configuration, and whether or not they'll open issues about these kinds of warnings.

only testing for it in places where it's obviously not going to occur

It wasn't obvious to me, since qt itself appeared to have abandoned the backport (maybe they opened a replacement backport or Canonical did their own backporting?)

It wasn't obvious to me, since qt itself appeared to have abandoned the backport

Fair enough. Given it was patched in 5.13, testing Fedora 34 + Qt 5.15.2 wasn't going to yield anything.

As for Ubuntu Focal + Qt 5.12.8:, my assumption was the same as yours (either different backport or Ubuntu patch), but I guess I should have just tested this myself first, as this still spews warnings...?

I guess NACK from me for now. The testing in the OP doesn't seem to have been done correctly.

Indeed, my tests were wrong (cannot figure out the reason of that now).