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

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:220321-enum changing 4 files +15 −12
  1. 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.

  2. fanquake added the label Build system on Mar 21, 2022
  3. 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.

  4. 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!

  5. hebasto force-pushed on Mar 21, 2022
  6. hebasto force-pushed on Mar 21, 2022
  7. 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.

  8. 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.

  9. promag commented at 10:37 PM on March 21, 2022: member

    Concept ACK Qt change.

  10. 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
  11. hebasto force-pushed on Mar 22, 2022
  12. hebasto commented at 8:02 AM on March 22, 2022: member

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

  13. 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?

  14. 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

  15. qt: Avoid potential -Wdeprecated-enum-enum-conversion warning acd98adaf1
  16. hebasto force-pushed on Mar 22, 2022
  17. hebasto renamed this:
    build: Add -Wdeprecated-enum-enum-conversion
    qt: Avoid potential -Wdeprecated-enum-enum-conversion warnings
    on Mar 22, 2022
  18. hebasto commented at 11:40 AM on March 22, 2022: member

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

  19. fanquake approved
  20. fanquake commented at 11:41 AM on March 22, 2022: member

    untested ACK acd98adaf1d83b71eda235c49d41a92f30c16313 - thanks.

  21. 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)

  22. 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.

  23. promag commented at 12:14 PM on March 22, 2022: member

    Code review ACK acd98adaf1d83b71eda235c49d41a92f30c16313.

  24. MarcoFalke merged this on Mar 22, 2022
  25. MarcoFalke closed this on Mar 22, 2022
  26. hebasto deleted the branch on Mar 22, 2022
  27. sidhujag referenced this in commit 3a42202953 on Mar 23, 2022
  28. DrahtBot locked this on Mar 22, 2023
Contributors
hebastofanquakeMarcoFalkepromag
Labels
Build system
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 09:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me