scripted-diff: Type-safe settings retrieval #31260
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31260. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Current status of this PR is that |
ryanofsky
force-pushed
the
pr/scripty
branch
2 times, most recently
from
47e30b4 to
d8a4a0a
Compare
|
Updated 416860f -> 47e30b4 ( |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
ryanofsky
force-pushed
the
pr/scripty
branch
from
d8a4a0a to
ad32a27
Compare
ryanofsky
force-pushed
the
pr/scripty
branch
from
ad32a27 to
534f971
Compare
ryanofsky
force-pushed
the
pr/scripty
branch
from
215f555 to
31fe4c6
Compare
There was a problem hiding this comment.
Updated 215f555 -> 31fe4c6 (pr/scripty.15 -> pr/scripty.16, compare) changing HelpFn arguments as suggested to make them more extensible and support querying default values
src/wallet/init_settings.h
Outdated
| using ConsolidateFeeRateSetting = common::Setting< | ||
| "-consolidatefeerate=<amt>", std::string, common::SettingOptions{.legacy = true}, | ||
| "The maximum feerate (in %s/kvB) at which transaction building may use more inputs than strictly necessary so that the wallet's UTXO pool can be reduced (default: %s)."> | ||
| ::HelpFn<[](const auto& fmt) { return strprintf(fmt, CURRENCY_UNIT, FormatMoney(DEFAULT_CONSOLIDATE_FEERATE)); }> |
There was a problem hiding this comment.
re: #31260 (comment)
This means that the
HelpFnwould (ideally) have a way to query the default value as well.
This is now implemented in the latest push.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
src/common/setting.h
Outdated
| //! which can be formatted at runtime using HelpFn/HelpArgs or | ||
| //! DefaultFn/DefaultArgs features (described below). | ||
| //! | ||
| //! Setting template class definiting-time Setting types which are |
There was a problem hiding this comment.
nit: is definiting-time a typo? maybe definition-time?
Avoid string concatenation and add missing namespace names so AddArg calls can be moved to header files in an upcoming scripted-diff.
Move constant declarations referenced in AddArg calls to headers so AddArg calls be moved to header files in an upcoming scripted-diff.
Drop hidden_args vector so all AddHiddenArgs calls can be replaced in upcoming scripted diff.
Linter changes to deal with _settings.h headers added in the next commit. In circular-dependencies.py, prevent _settings.h headers from being included transitively. Add a new check that makes it an error to include _settings.h from another header file, rather than a .cpp file. Use this fact to avoid errors about .cpp files including _settings.h files being circularly dependent based on each other. In lint-format-strings.py, skip _settings.h because they contain lines like strformat(fmt, ...) where fmt is a compile time constant that is checked at compile time by the compiler and can't be checked by this linter. In check-doc.py update test to look for Setting::Get and Setting::Register instead of GetArg and AddArg.
…egister / Get calls
…/ Get calls This commit is a pure refactoring and does not change behavior in any way. -BEGIN VERIFY SCRIPT- python contrib/devtools/reg-settings.py git add -N src/bench/bench_bitcoin_settings.h src/bitcoin-tx_settings.h src/bitcoin-util_settings.h src/bitcoin-wallet_settings.h src/chainparamsbase_settings.h src/common/args_settings.h src/init/common_settings.h src/init_settings.h src/qt/bitcoin_settings.h src/test/argsman_tests_settings.h src/test/logging_tests_settings.h src/wallet/init_settings.h src/dummywallet_settings.h src/qt/test/optiontests_settings.h src/test/getarg_tests_settings.h git rm contrib/devtools/reg-settings.py -END VERIFY SCRIPT-
|
Rebased 31fe4c6 -> b396835 ( |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
This PR changes the way settings are registered and retrieved to provide more compile-time safety. Currently settings are registered and retrieved like:
But this is not ideal because nothing checks that setting names are spelled correctly, or that default values (
BITCOIN_PID_FILENAME) are used consistently in the help string and retrieval sites, or that settings are retrieved with consistent types (bool, int, string, path, or list). This PR addresses these issues by adding setting declarations which allow settings to be registered and retrieved like:Suggestions for review
All the real changes in this PR are in the last scripted-diff commit:
31fe4c6f9946scripted-diff: Replace AddArgs / GetArgs calls with Setting Register / Get callsIf you take a few minutes to look at the changes applied in this scripted-diff commit, you'll understand everything this PR is doing. A good place to start looking around in this commit is the generated
src/init_settings.hfile which declares the settings that get registered in src/init.cpp. Then you can look at the surrounding diffs and see they are just replacingAddArgandGetArgcalls.The other notable commit is second commit, which implements the
Settingclass:0434e4d86f71common: Add Setting class to support typed SettingsThe other commits do minor things like moving code or updating linters. The python script that implements the scripted diff is a temporary artifact that gets deleted. The python script is complicated because it does things like parsing c++ code to extract help strings, and figuring out the right types to declare settings with so code compiles. But the entire scope of the script is to (1) generate Setting type definitions, (2) add #includes, and (3) replace
AddArg()calls withRegister()calls andGetArg()calls withGet()calls. So there is not much the script can actually do wrong without triggering build and test failures.Extensions
This PR only adds the ability to declare individual settings with built-in types. It doesn't provide any new runtime behavior, but a branch in issue #22978 extends the
Settingclass implemented here to support runtime setting validation, additional types likestd::variant, additional conversion options, custom types, custom validation, and groups of settings declared as options structs.This change is mostly orthogonal to #16545. #16545 only provides runtime type checking while this PR only provides compile-time checking with no new runtime behavior. But this change does allow a nicer way of declaring types in #16545, using c++ types like
intinstead of flags likeALLOW_INT, orstd::vector<std::string>instead ofALLOW_STRING | ALLOW_LIST.