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

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: member

   • #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. MarcoFalke force-pushed on Sep 24, 2018
6. MarcoFalke force-pushed on Sep 26, 2018
7. MarcoFalke force-pushed on Sep 26, 2018
8. MarcoFalke force-pushed on Sep 26, 2018
9. MarcoFalke force-pushed on Sep 26, 2018
10. MarcoFalke force-pushed on Sep 26, 2018
11. MarcoFalke force-pushed on Sep 26, 2018
12. 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
13. MarcoFalke force-pushed on Sep 26, 2018
14. MarcoFalke force-pushed on Sep 26, 2018
15. chainparams: Add type safe enum class ChainType d341856cf0
16. 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
17. MarcoFalke force-pushed on Sep 26, 2018
18. Use non-throwing chain selection calls where possible fab4f8a196
19. MarcoFalke force-pushed on Sep 26, 2018
20. MarcoFalke force-pushed on Sep 26, 2018
21. MarcoFalke force-pushed on Sep 26, 2018
22. 
    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.

23. 
    DrahtBot commented at 5:00 am on September 28, 2018: member

    Coverage	Change (pull 14309)	Reference (master)
    Lines	+0.0423 %	87.0361 %
    Functions	+0.0788 %	84.1130 %
    Branches	+0.0236 %	51.5451 %

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

25. qa: Add tests for invalid chain selections 8c0eabf5b5
26. MarcoFalke force-pushed on Oct 21, 2018
27. 
    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).
28. MarcoFalke closed this on Oct 21, 2018

    ---

29. MarcoFalke deleted the branch on Oct 21, 2018
30. MarcoFalke locked this on Sep 8, 2021

Contributors
MarcoFalke practicalswift DrahtBot jtimon

Labels
Refactoring