Use non-throwing type-safe ChainType where possible #14309

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:Mf1809-chainType changing 33 files +276 −130
  1. MarcoFalke commented at 5:05 PM on September 24, 2018: member

    Currently we use string constants to select different chain rules (e.g "main" or "test"). The obvious downsides are that any function that accepts such a string needs to check if it is one of the known ones and return a runtime error when not found.

    This check can be done at compile time by using a switch statement on an enum class.

    To not refactor all the existing code, this first introduces the type-safe enum class ChainType and then runs a scripted diff on all instances where we can benefit from the new class. I intend to remove the throwing and now deprecated methods that pass around the string in a future patch.

  2. MarcoFalke added the label Refactoring on Sep 24, 2018
  3. practicalswift commented at 5:37 PM on September 24, 2018: contributor

    Concept ACK

    Nice!

  4. DrahtBot commented at 8:39 PM on September 24, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->Reviewers, this pull request conflicts with the following ones:

    • #14494 (Error if # is used in rpcpassword in conf by MeshCollider)
    • #14489 (refactor: Drop boost::thread and boost::chrono by ken2812221)
    • #14437 (Refactor: Start to separate wallet from node by ryanofsky)
    • #14350 (Add WalletLocation class by promag)
    • #14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
    • #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
    • #13216 ([Qt] implements concept for different disk sizes on intro by marcoagner)
    • #12557 ([WIP] 64 bit iOs device support by Sjors)
    • #8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

    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.

  5. DrahtBot cross-referenced this on Sep 24, 2018 from issue Don't edit Chainparams after initialization by jtimon
  6. DrahtBot cross-referenced this on Sep 24, 2018 from issue refactor: Fix the chainparamsbase -> util circular dependency by Empact
  7. MarcoFalke force-pushed on Sep 24, 2018
  8. DrahtBot cross-referenced this on Sep 24, 2018 from issue [WIP] 64 bit iOS device support by Sjors
  9. DrahtBot cross-referenced this on Sep 24, 2018 from issue Add BitcoinApplication & RPCConsole tests by ryanofsky
  10. MarcoFalke force-pushed on Sep 26, 2018
  11. MarcoFalke force-pushed on Sep 26, 2018
  12. MarcoFalke force-pushed on Sep 26, 2018
  13. DrahtBot cross-referenced this on Sep 26, 2018 from issue util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool by practicalswift
  14. MarcoFalke force-pushed on Sep 26, 2018
  15. MarcoFalke force-pushed on Sep 26, 2018
  16. MarcoFalke force-pushed on Sep 26, 2018
  17. MarcoFalke renamed this:
    scripted-diff: Use non-throwing type-safe ChainType where possible
    Use non-throwing type-safe ChainType where possible
    on Sep 26, 2018
  18. MarcoFalke force-pushed on Sep 26, 2018
  19. MarcoFalke force-pushed on Sep 26, 2018
  20. chainparams: Add type safe enum class ChainType d341856cf0
  21. scripted-diff: Use non-throwing type-safe ChainType where possible
    -BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/(Create(Base)?ChainParams)\(C(Base)?ChainParams::/\1(ChainType::/g' $(git grep -l --extended-regexp 'Create(Base)?ChainParams')
sed -i --regexp-extended -e 's/((S|s)elect(Base)?Params)\(C(Base)?ChainParams::/\1(ChainType::/g' $(git grep -l --extended-regexp 'elect(Base)?Params')
-END VERIFY SCRIPT-
    731037e6cd
  22. MarcoFalke force-pushed on Sep 26, 2018
  23. Use non-throwing chain selection calls where possible fab4f8a196
  24. MarcoFalke force-pushed on Sep 26, 2018
  25. MarcoFalke force-pushed on Sep 26, 2018
  26. MarcoFalke force-pushed on Sep 26, 2018
  27. practicalswift commented at 7:36 AM on September 27, 2018: contributor

    What about adding the NODISCARD/[[nodiscard]] attribute to bool ParseChainType(const std::string& str, ChainType& chain) to guarantee that all future callers make use of the return code?

    See #13815 for NODISCARD which enables [[nodiscard]] (or equivalent) without having to wait for C++17.

  28. DrahtBot cross-referenced this on Sep 27, 2018 from issue Refactor: separate wallet from node by ryanofsky
  29. DrahtBot cross-referenced this on Sep 27, 2018 from issue Multiprocess bitcoin by ryanofsky
  30. DrahtBot commented at 5:00 AM on September 28, 2018: contributor

    <!--32850dd3fdea838b4049e64f46995ea2-->

    Coverage Change (pull 14309) Reference (master)
    Lines +0.0423 % 87.0361 %
    Functions +0.0788 % 84.1130 %
    Branches +0.0236 % 51.5451 %
  31. DrahtBot cross-referenced this on Sep 28, 2018 from issue [Qt] implements concept for different disk sizes on intro by marcoagner
  32. DrahtBot cross-referenced this on Sep 28, 2018 from issue Add WalletLocation class by promag
  33. DrahtBot cross-referenced this on Oct 9, 2018 from issue Refactor: Start to separate wallet from node by ryanofsky
  34. DrahtBot cross-referenced this on Oct 20, 2018 from issue Error if # is used in rpcpassword in conf by meshcollider
  35. DrahtBot cross-referenced this on Oct 20, 2018 from issue refactor: Drop boost::thread and boost::chrono by ken2812221
  36. DrahtBot cross-referenced this on Oct 20, 2018 from issue Testchains: Introduce custom chain whose constructor... by jtimon
  37. jtimon commented at 2:55 PM on October 20, 2018: contributor

    why not a macro or a simple const string in chainbaseparams.h ? I think the problem I see is the string cannot be used in certain place now (ie chainparams.cpp for setting strNetworkID and in qt/networkstyle.cpp for setting network_styles) only comes from the fact that the strings are not constants but static attributes in the class.

    The errors thrown are only thrown during initialization and some of them would disappear with #8994 I'm not sure this adds much value.

  38. qa: Add tests for invalid chain selections 8c0eabf5b5
  39. MarcoFalke force-pushed on Oct 21, 2018
  40. MarcoFalke commented at 1:50 AM on October 21, 2018: member

    Closing for now, but I think in the longer term we should get rid of exceptions (especially during initialization, where we currently use a string to explicitly pass up errors).

  41. MarcoFalke closed this on Oct 21, 2018
  42. MarcoFalke deleted the branch on Oct 21, 2018
  43. MarcoFalke cross-referenced this on Feb 1, 2021 from issue refactor: don't throw in GetChainName() by fanquake
  44. bitcoin locked this on Sep 8, 2021
Contributors
MarcoFalkepracticalswiftDrahtBotjtimon
Labels
Refactoring
Linked (view graph)
#21039 refactor: don't throw in GetChainName()
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-05-19 06:54 UTC