init: settings, do not load auto-generated warning msg #29301
Conversation
The settings warning message is meant to be used only to discourage users from modifying the file manually. Therefore, there is no need to keep it in memory.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
|
ACK 987a1b5 |
| @@ -31,6 +31,9 @@ enum class Source { | |||
| CONFIG_FILE_DEFAULT_SECTION | |||
| }; | |||
|
|
|||
| // Json object key for the auto-generated warning comment | |||
| const std::string SETTINGS_WARN_MSG_KEY{"_warning_"}; | |||
There was a problem hiding this comment.
In commit "init: settings, do not load auto-generated warning msg" (987a1b5)
Not sure, but maybe this can be changed to std::string_view to avoid the need to allocate memory before main() runs.
There was a problem hiding this comment.
In commit "init: settings, do not load auto-generated warning msg" (987a1b5)
Not sure, but maybe this can be changed to std::string_view to avoid the need to allocate memory before main() runs.
As the settings map take strings for keys, wouldn't this change mean that we would need to static cast string_view to string to not create a copy during the push/erase methods call?
There was a problem hiding this comment.
re: #29301 (comment)
As the settings map take strings for keys, wouldn't this change mean that we would need to static cast string_view to string to not create a copy during the push/erase methods call?
The idea is that just that it is noticeable when programs are slow to start, so you usually want to reduce the amount of code that needs to run before main(), even if it means code that runs later but is less sensitive might be slower. In this case there is not even a need for a tradeoff, though since univalue could take string_view arguments. Just explaining the suggestion though, this is not important
There was a problem hiding this comment.
Sure, sounds good @ryanofsky. Thanks for coming back to this!
There was a problem hiding this comment.
Seems fine. Gets rid of the pointless 2024-01-31T10:25:34Z Ignoring unknown rw_settings value _warning_ and
2024-01-31T10:25:34Z Setting file arg: _warning_ = "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten." output.
Fixes #29144 (comment).
The settings warning message is meant to be used only to discourage users from
modifying the file manually. Therefore, there is no need to keep it in memory.