gui: Make RPCConsole::TabTypes an enum class #17105

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-10-fix-warning changing 3 files +16 −14
  1. promag commented at 11:04 PM on October 10, 2019: member

    This change makes the compiler emit a warning/error if a missing enum value is not handled. See also #17134.

  2. fanquake added the label GUI on Oct 10, 2019
  3. promag commented at 11:05 PM on October 10, 2019: member

    Thanks @sipa for reporting.

    This is one of the various ways of fixing this. @luke-jr mentioned in IRC:

    add assert(false) after the switch?

  4. luke-jr commented at 11:15 PM on October 10, 2019: member

    I guess we don't really want to assert if we miss the warning.

    utACK

  5. MarcoFalke commented at 11:24 PM on October 10, 2019: member

    What about switching this to an enum class, so that the compiler can warn if not all cases are covered?

    See git grep 'compiler can warn'

  6. promag commented at 11:42 PM on October 10, 2019: member

    That would prevent the warning?

  7. MarcoFalke commented at 11:51 PM on October 10, 2019: member

    No, you can trivially check by removing a case and compiling

  8. promag commented at 11:56 PM on October 10, 2019: member

    What about switching this to an enum class

    Then I guess that's an improvement for a new PR.

  9. promag commented at 12:52 AM on October 11, 2019: member

    @MarcoFalke decided to include the change, thanks!

  10. DrahtBot commented at 1:03 AM on October 11, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17128 (gui: remove unnecessary shortcuts in bitcoingui files by GChuf)
    • #17096 (gui: rename debug window by Zero-1729)

    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.

  11. in src/qt/rpcconsole.h:66 in 4fd5d52b7e outdated 
      66 | +    enum class TabTypes {
  67 | +        INFO = 0,
  68 | +        CONSOLE = 1,
  69 | +        GRAPH = 2,
  70 | +        PEERS = 3
  71 |      };
    hebasto commented at 3:13 AM on October 11, 2019:

    Why not just

    enum class TabTypes { INFO, CONSOLE, GRAPH, PEERS };

    ?

    promag commented at 6:42 AM on October 11, 2019:

    Could be, but note that almost all are multi-line - which is better for adding comments - so I'm going to leave as it is.

  12. hebasto commented at 3:14 AM on October 11, 2019: member

    Concept ACK.

  13. hebasto approved
  14. hebasto commented at 8:49 AM on October 11, 2019: member

    ACK 4fd5d52b7eb574b32f20ca82174855e27470677c, tested on Linux Mint 19.2.

    Also, can confirm that related compiler (gcc 7.4.0) warning has gone.

  15. in src/qt/rpcconsole.cpp:1287 in 4fd5d52b7e outdated 
    1288 | +    case TabTypes::INFO: return QKeySequence(Qt::CTRL + Qt::Key_I);
1289 | +    case TabTypes::CONSOLE: return QKeySequence(Qt::CTRL + Qt::Key_T);
1290 | +    case TabTypes::GRAPH: return QKeySequence(Qt::CTRL + Qt::Key_N);
1291 | +    case TabTypes::PEERS: return QKeySequence(Qt::CTRL + Qt::Key_P);
1292 |      }
1293 | +    return QKeySequence();
    MarcoFalke commented at 2:30 PM on October 11, 2019:

    This code should never execute

        assert(false);
    promag commented at 2:45 PM on October 11, 2019:

    There's only warnings for unhandled cases:

    qt/rpcconsole.cpp:1281:13: warning: enumeration value 'INFO' not handled in switch [-Wswitch]
    switch (tab_type) {
            ^
qt/rpcconsole.cpp:1281:13: note: add missing switch cases
    switch (tab_type) {
            ^
1 warning generated.

    So why rely on a runtime assertion?

    MarcoFalke commented at 2:48 PM on October 11, 2019:

    This is a compiler error, i.e. the compilation will not succeed. See #16424 (comment)

    MarcoFalke commented at 2:48 PM on October 11, 2019:

    I think we need more documentation around this. I am repeating the same points every couple of weeks.

    promag commented at 2:54 PM on October 11, 2019:

    Oh sorry then (have to check why it doesn't fail here). I'll change as you suggest.

    MarcoFalke commented at 5:49 PM on October 11, 2019:

    Have you done --enable-werror?

    promag commented at 8:41 AM on October 12, 2019:

    No, that wasn't set, thanks 👍

  16. in src/qt/rpcconsole.h:65 in 4fd5d52b7e outdated 
      65 | -        TAB_PEERS = 3
  66 | +    enum class TabTypes {
  67 | +        INFO = 0,
  68 | +        CONSOLE = 1,
  69 | +        GRAPH = 2,
  70 | +        PEERS = 3
    MarcoFalke commented at 2:30 PM on October 11, 2019: 
            PEERS,

    No need to manually enumerate those, when the compiler does it for you

  17. promag force-pushed on Oct 12, 2019
  18. in src/qt/rpcconsole.cpp:1285 in 1d54a1ee35 outdated 
    1286 | -    case TAB_GRAPH: return QKeySequence(Qt::CTRL + Qt::Key_N);
1287 | -    case TAB_PEERS: return QKeySequence(Qt::CTRL + Qt::Key_P);
1288 | +    case TabTypes::INFO: return QKeySequence(Qt::CTRL + Qt::Key_I);
1289 | +    case TabTypes::CONSOLE: return QKeySequence(Qt::CTRL + Qt::Key_T);
1290 | +    case TabTypes::GRAPH: return QKeySequence(Qt::CTRL + Qt::Key_N);
1291 | +    case TabTypes::PEERS: return QKeySequence(Qt::CTRL + Qt::Key_P);
    hebasto commented at 8:36 AM on October 14, 2019:

    Could add the same comment like elsewhere?

    // no default case, so the compiler can warn about missing cases

    Refs:

  19. fanquake referenced this in commit dcc640811c on Oct 14, 2019
  20. laanwj commented at 6:24 AM on October 15, 2019: member

    Please change the PR title and/or commit message to match the actual changes here.

  21. gui: Make RPCConsole::TabTypes an enum class 8019b6b150
  22. promag renamed this:
    gui: Add missing return value in RPCConsole::tabShortcut
    gui: Make RPCConsole::TabTypes an enum class
    on Oct 15, 2019
  23. promag force-pushed on Oct 15, 2019
  24. promag commented at 12:53 PM on October 15, 2019: member

    @hebasto @laanwj done.

  25. hebasto approved
  26. hebasto commented at 12:54 PM on October 15, 2019: member

    re-ACK 8019b6b150ff7444195a238470414c9deec5bf74

  27. MarcoFalke commented at 2:45 PM on October 15, 2019: member

    unsigned ACK 8019b6b150ff7444195a238470414c9deec5bf74

  28. fanquake approved
  29. fanquake commented at 7:43 PM on October 15, 2019: member

    ACK 8019b6b150ff7444195a238470414c9deec5bf74

  30. fanquake referenced this in commit eb292af309 on Oct 15, 2019
  31. fanquake merged this on Oct 15, 2019
  32. fanquake closed this on Oct 15, 2019
  33. MarkLTZ referenced this in commit c364638ce6 on Nov 17, 2019
  34. jasonbcox referenced this in commit 95c8ebac04 on Oct 28, 2020
  35. DrahtBot locked this on Dec 16, 2021
Contributors
promagluke-jrMarcoFalkeDrahtBothebastolaanwjfanquake
Labels
GUI
Linked (view graph)
#17134 doc: Add switch on enum example to developer notes
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-16 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me