This issue exists to explore ways of improving the ArgsManager interface to be more convenient and add compile-time type safety.
#16545 adds runtime type checking for ArgsManager options, but it isn’t changing ArgsManager AddArg / GetArg interface, only adding new flags and behaviors to the existing interface, so it is not taking advantage of C++’s type safety to enforce at compile time that options are always retrieved with the same types they were registered with. A branch that implements this feature is being developed at:
And is described in (comment) and (comment).
Planned additional features are:
- Dropping dependency on #16545 and making new interface backwards compatible with existing arguments.
- Writing a scripted diff to port all current ArgsManager AddArg and GetArg calls to use the new interface and be backwards compatible, without changing any behavior
- Adding helpers to read arguments directly into Options structs like BlockManager::Options MemPoolOptions without needing to list types or struct members more than once.
- Adding support for custom types (enums, structs, and classes) so Options structs are not limited to reading bool, int, and string options.
- Adding documentation about the implementation and how to use the interface.
This issue description has been edited since the issue was opened. The original description was:
Originally posted by @ryanofsky in #22766 (review)
The way I would like range checking to work in the future would be to rely more on C++ types and std::numeric_limits. The idea is arguments would be registered using explicit C++ types:
0const Setting<int> SETTING_myint("-myint", "description");
1const Setting<std::string> SETTING_mystring("-mystring", "description");
2const Setting<std::vector<std::string>> SETTING_mylist("-mylist", "description");
3const Setting<std::optional<std::uint16_t>> SETTING_myopt("-myopt", "description");
4const Setting<SettingsValue> SETTING_mylegacy("-mylegacy", "description");
5
6void RegisterArgs(ArgsManager& args)
7{
8 args.Register({SETTING_myint, SETTING_mystring, SETTING_mylist, SETTING_myopt, SETTING_mylegacy});
9}
and then they could be retrieved in a type safe way:
0args.Get(SETTING_myint); // returns int
1args.Get(SETTING_mystring); // returns std::string
2args.Get(SETTING_mylist); // returns std::vector<std::string>
3args.Get(SETTING_myopt); // returns std::optional<uint16_t>
4args.GetArg/GetArgs/GetIntArg/GetBoolArg(SETTING_mylegacy); // returns requested type
To get to this point, this PR cleans up existing misused flags and misnamed functions. PR #16545 adds type validation and runtime semantics without changing the ArgsManager API, and a followup PR can improve the API and update call sites without changing the semantics. (There is a direct correspondence between the ALLOW_ flags from #16545 and the useful C++ settings types bool/int/std::string/std::optional/std::variant/std::vector)
ajtowns #22766 (review)
I think I finally figured out how you can go a little bit further than the above, ending up with something like:
0struct NetSettings
1{
2 int64_t blockreconstructionextratxn;
3 int64_t maxorphantx;
4 bool capturemessages;
5
6 template<typename C, typename... Args>
7 static inline void F(Args&... args) {
8 return C::Do(args...,
9 C::Defn( &NetSettings::blockreconstructionextratxn, "-blockreconstructionextratxn=<n>", strprintf("Extra transactions to keep in memory for compact block reconstructions (default: %u)", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS, DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN ),
10 C::Defn( &NetSettings::maxorphantx, "-maxorphantx=<n>", strprintf("Keep at most <n> unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS, DEFAULT_MAX_ORPHAN_TRANSACTIONS ),
11 C::Defn( &NetSettings::capturemessages, "-capturemessages", "Capture all P2P messages to disk", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST, false )
12 );
13 }
14};
15
16void RegisterNetProcessingArgs(ArgsManager& args)
17{
18 SettingsRegister<NetSettings>::Register(args);
19}
20
21NetSettings GetNetSettings(const ArgsManager& args)
22{
23 return SettingsRegister<NetSettings>::Get(args);
24}
25
26class PeerManagerImpl
27{
28private:
29 const NetSettings m_settings;
30 PeerManagerImpl(..., const ArgsManager& args) : m_settings{GetNetSettings(args)), ... { ... }
31 ...
32};
The idea being that this way:
- it can infer the argument type directly from the type of the struct member so that you can’t accidentally specify different types between
args.AddArg<int>andGet<bool> - that the settings are const at runtime so can be accessed without any additional locks
- you only have to access the ArgsManager (and do string parsing) at setup time
- you don’t have to make up lots of new names for everything or add too much boilerplate
Branch built on top of this PR that has the above working at https://github.com/ajtowns/bitcoin/tree/202109-settings_struct
