qt: Avoid potential -Wdeprecated-enum-enum-conversion warnings

hebasto commented at 10:18 am on March 21, 2022: member

This PR is related to bitcoin/bitcoin#24169. It adjusts code in order to avoid -Wdeprecated-enum-enum-conversion warnings instead of disabling them.

Could be tested with gcc 11.2.

fanquake added the label Build system on Mar 21, 2022

in configure.ac:452 in ae4cd25b97 outdated

448@@ -449,6 +449,7 @@ if test "$CXXFLAGS_overridden" = "no"; then
449                         [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])])
450   AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"], [], [$CXXFLAG_WERROR])
451   AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wimplicit-fallthrough"], [], [$CXXFLAG_WERROR])
452+  AX_CHECK_COMPILE_FLAG([-Wdeprecated-enum-enum-conversion], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdeprecated-enum-enum-conversion"], [], [$CXXFLAG_WERROR])

fanquake commented at 10:22 am on March 21, 2022:

Looks like his is enabled by default when targeting C++20 in GCC, and by default in Clang, so I don’t think there’s a need to add this explicitly.

MarcoFalke commented at 10:23 am on March 21, 2022:

I presume it is not enabled by default in C++17, which is used to compile Bitcoin Core (and will be for the coming months/years)?

fanquake commented at 10:29 am on March 21, 2022:

It’s enabled by default in Clang, which should be good enough coverage for us. In GCC, when targetting pre-C++20, it’s enabled by -Wenum-conversion, which is part of -Wextra, which we already enable. Although in that case it may only be applied to C code.

hebasto commented at 10:44 am on March 21, 2022:

I presume it is not enabled by default in C++17

Correct.

MarcoFalke commented at 10:56 am on March 21, 2022:

It’s enabled by default in Clang, which should be good enough coverage for us.

For me it is only enabled with c++20. Do you have steps to reproduce?

fanquake commented at 11:10 am on March 21, 2022:

Do you have steps to reproduce?

No. I assumed Clang might be enabling with any standard, given they don’t specify otherwise, like GCC does. However, I’m still a NACK on adding this. It’s already enabled by default when it’s needed, it’s only available on fairly “new” compilers, so having everyone building with older ones check for availability is pointless, and I’d prefer that our configure checks don’t return to being a grab-bag of checks for random flags. In any case, given the only existence of warning producing code, are a few instances in the GUI, I’d say the likelihood of any warning producing code being introduced is a also very low.

hebasto commented at 11:18 am on March 21, 2022:

I’m still a NACK on adding this.

An additional minor pro for adding this line is simplicity of testing this PR by reviewers using gcc.

fanquake commented at 11:24 am on March 22, 2022:

An additional minor pro for adding this line is simplicity of testing this PR by reviewers using gcc.

I don’t really think we should be adding warning flags to our build system for the sake of making testing PRs easier. If anyone wants to test this change, they can do ./configure CXXFLAGS="-Wdeprecated-enum-enum-conversion" using a compiler that supports the option.

hebasto commented at 11:41 am on March 22, 2022:

Updated.

in src/qt/bitcoingui.cpp:255 in ae4cd25b97 outdated

251@@ -251,28 +252,28 @@ void BitcoinGUI::createActions()
252     overviewAction->setStatusTip(tr("Show general overview of wallet"));
253     overviewAction->setToolTip(overviewAction->statusTip());
254     overviewAction->setCheckable(true);
255-    overviewAction->setShortcut(QKeySequence(Qt::ALT + Qt::Key_1));
256+    overviewAction->setShortcut(QKeySequence(tr("Alt+1")));

MarcoFalke commented at 10:26 am on March 21, 2022:

Isn’t tr used to mark messages for translation? If yes, why is this needed here? If this is needed to convert to QString, why not call QString{} instead?

hebasto commented at 10:43 am on March 21, 2022:

Isn’t tr used to mark messages for translation?

Yes, it is.

If yes, why is this needed here? If this is needed to convert to QString, why not call QString{} instead?

No need for translation here. Updated in the recent push.

Thanks!

hebasto force-pushed on Mar 21, 2022

in src/qt/bitcoingui.cpp:255 in 0fea3846d0 outdated

251@@ -251,28 +252,28 @@ void BitcoinGUI::createActions()
252     overviewAction->setStatusTip(tr("Show general overview of wallet"));
253     overviewAction->setToolTip(overviewAction->statusTip());
254     overviewAction->setCheckable(true);
255-    overviewAction->setShortcut(QKeySequence(Qt::ALT + Qt::Key_1));
256+    overviewAction->setShortcut(QKeySequence(QString{"Alt+1"}));

promag commented at 10:32 pm on March 21, 2022:

0fea3846d0d6ec69dadd8bf043afd3c25971d07f

nit, here and below, use QStringLiteral, see https://doc.qt.io/qt-5/qstring.html#more-efficient-string-construction

hebasto commented at 8:03 am on March 22, 2022:

Thanks! Updated.

in src/qt/guiutil.cpp:418 in 0fea3846d0 outdated

414@@ -414,7 +415,7 @@ void bringToFront(QWidget* w)
415 
416 void handleCloseWindowShortcut(QWidget* w)
417 {
418-    QObject::connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::Key_W), w), &QShortcut::activated, w, &QWidget::close);
419+    QObject::connect(new QShortcut(QKeySequence(QShortcut::tr("Ctrl+W")), w), &QShortcut::activated, w, &QWidget::close);

promag commented at 10:36 pm on March 21, 2022:

0fea3846d0d6ec69dadd8bf043afd3c25971d07f

QObject::tr()

hebasto commented at 8:11 am on March 22, 2022:

The initial idea was to put this translation into the context named “QShortcut”, rather “QObject”. But context name does not go from src/qt/locale/bitcoin_en.ts into the Transifex environment. So accepting your suggestion.

promag commented at 10:37 pm on March 21, 2022: member

Concept ACK Qt change.

qt: Use human-readable strings in preference to hard-coded integers

This is recommended by Qt docs.
See: https://doc.qt.io/qt-5/qkeysequence.html#details

Also this change avoids -Wdeprecated-enum-enum-conversion warnings.

d8641f04e4

hebasto force-pushed on Mar 22, 2022

hebasto commented at 8:02 am on March 22, 2022: member

Updated 38cffbb301e8d2ddf8dbc12afee99e76d8b54ebd -> 103b8dd8b89c772b7f6d097383e83d742b5ed55c (pr24624.03 -> pr24624.04, diff):

addressed @promag’s comments

MarcoFalke commented at 8:13 am on March 22, 2022: member

Can you explain why any of this needs translation? Is it displayed anywhere to the user?

hebasto commented at 8:16 am on March 22, 2022: member

Can you explain why any of this needs translation? Is it displayed anywhere to the user?

Screenshot from 2022-03-22 09-16-01

qt: Avoid potential -Wdeprecated-enum-enum-conversion warning acd98adaf1

hebasto force-pushed on Mar 22, 2022

hebasto renamed this:
~~build: Add -Wdeprecated-enum-enum-conversion~~
qt: Avoid potential -Wdeprecated-enum-enum-conversion warnings
on Mar 22, 2022

hebasto commented at 11:40 am on March 22, 2022: member

Updated 103b8dd8b89c772b7f6d097383e83d742b5ed55c -> acd98adaf1d83b71eda235c49d41a92f30c16313 (pr24624.04 -> pr24624.05, diff):

addressed @fanquake’s comment

fanquake approved

fanquake commented at 11:41 am on March 22, 2022: member

untested ACK acd98adaf1d83b71eda235c49d41a92f30c16313 - thanks.

MarcoFalke commented at 11:47 am on March 22, 2022: member

Approach ACK acd98adaf1d83b71eda235c49d41a92f30c16313

For testing locally, #24169 will need to be merged. Edit: Or ./configure CXXFLAGS="-Wdeprecated-enum-enum-conversion", according to #24624 (review)

fanquake commented at 11:48 am on March 22, 2022: member

For testing locally, #24169 will need to be merged.

Why? You can just do ./configure CXXFLAGS="-Wdeprecated-enum-enum-conversion" with a supporting compiler.

promag commented at 12:14 pm on March 22, 2022: member

Code review ACK acd98adaf1d83b71eda235c49d41a92f30c16313.

MarcoFalke merged this on Mar 22, 2022

MarcoFalke closed this on Mar 22, 2022

hebasto deleted the branch on Mar 22, 2022

sidhujag referenced this in commit 3a42202953 on Mar 23, 2022

DrahtBot locked this on Mar 22, 2023

qt: Avoid potential -Wdeprecated-enum-enum-conversion warnings #24624