RFC: Replacing ArgsManager enum with enum class #22977

issue ryanofsky openend this issue on September 15, 2021
  1. ryanofsky commented at 1:40 am on September 15, 2021: contributor

    Originally posted by @kiminuo in #22766 (review)

    I wonder whether enum class Flags : uint32_t may be used here as Jon did in https://github.com/bitcoin/bitcoin/pull/21506/ (guideline). Would it make sense to you in this or a follow-up PR?


    ryanofsky #22766 (review)

    This could be fine, but I’m not sure what the specific advantage of doing it would be in this case (not sure what ways it may be safer). I do think it’s basically tangential in any case, and could be better addressed in a standalone PR.


    kiminuo #22766 (review)

    I think the rationale in the guideline makes sense and that’s why I suggested it. There was no specific advatange I had in mind. Anyway, it may (not) be a good follow up PR.


    ajtowns #22766 (review)

    See NetPermissionsFlags in net_permissions.h for comparison; it gives you lots of namespacing (so you’d be writing ArgsManager::Flags::ALLOW_BOOL everywhere) and means you can’t just treat them as ints (so bitwise or’ing things together means you have to declare your own operator)


    jonatack #22766 (review)

    Yes, two sides of a coin, I suppose.

    Namespacing can allow shortening the enumerator names, since they no longer need to be unique in global namespace (and can therefore no longer need to be shouty “there can only be one”), e.g. ArgsManager::ALLOW_STRING could be ArgsManager::Flags::string. Whether that naming is better may be personal taste, but getting them out of global space seems good.

    Using an enum class permits defining allowed operations: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-oper and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class (“minimize surprises: traditional enums convert to int too readily”). Changing NetPermissionFlags to an enum class found a bug related to this reason.

  2. ryanofsky added the label Brainstorming on Sep 15, 2021
  3. ryanofsky commented at 12:46 pm on September 25, 2023: contributor
    Closing. Seems like switching from enum to enum class would require adding a lot of type casts, in any case. In longer run, flags could be replaced with c++ types using an approach like ones discussed in #22978
  4. ryanofsky closed this on Sep 25, 2023

  5. bitcoin locked this on Sep 24, 2024


ryanofsky

Labels
Brainstorming


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: 2024-11-23 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me