test, refactor: Embedded ASmap selected preparatory work #33026
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/33026. 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. |
|
tACK 942d95d I thoroughly reviewed and understood all code changes. I ran the tests locally and they all passed. |
b59dc21 doc: Fix typos in asmap README (nervana21) Pull request description: This minor PR fixes some spelling mistakes found while reviewing #33026. ACKs for top commit: fanquake: ACK b59dc21 Tree-SHA512: e76f7f97c10f3e506d024da0cbf804f4975cf07f31f0dd0abad6fcb97a5fa1032087459dba46de3715f6275d47e2fde4d8db3d38341540110d87fd5669855359
| @@ -273,7 +273,7 @@ fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) | |||
| { | |||
| if (IsArgNegated(arg)) return fs::path{}; | |||
| std::string path_str = GetArg(arg, ""); | |||
| if (path_str.empty()) return default_value; | |||
| if (path_str.empty() || path_str == "1") return default_value; | |||
There was a problem hiding this comment.
In 74932c6: Seems a bit awkaward to add an (undocumented) special case here for asmap? Can't we just fail if the value given isn't a valid path, or stop supporting this behaviour? cc @ryanofsky
There was a problem hiding this comment.
I can drop this is other reviewers agree that it's confusing. I personally think this is the more intuitive behavior for path args independently of asmap because our bool args all work with both -foo and -foo=1 afaik. If the user wants to use -asmap (or any path arg) as a bool then I think they would expect it to work the same as the other bool params. Fwiw, I discovered this because I wrote a functional test that covered -asmap=1 and I actually expected it work, only after it didn't I added this.
942d95d to
eb04710
Compare
Otherwise -asmap=1, for example, would be interpreted as a path "1".
eb04710 to
734737b
Compare
|
ACK 734737b |
This contains four commits from #28792 that can be easily reviewed and merged independently. I hope splitting this change off can make this part move a bit faster and reduce frequency of needed rebases for #28792.
The commits in order:
util/asmap.cpp, I added this while touching the rest of the file all over anyway.GetPathArgfunction when an path arg is given "1" (for example-asmap=1), which would be interpreted as a path but should be interpreted as using the are without a parameter which activates the fallback. There is no explicit test added here because this is tested implicitly in the tests added in Embed default ASMap as binary dump header file #28792.AutoFile::sizehelper function