as above
edit: also renamed a variable that used Hungarian notation
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rSTAGINGc8d1fedde875: minor refactor to use ranged_for, auto and const-ness
rABCc8d1fedde875: minor refactor to use ranged_for, auto and const-ness
created a minor cpp unit test which replicated the desired behavior in the code. that being insufficient, as soon as my node syncs I'll prune it, set a breakpoint and verify correct behavior
edit: To check for correct behavior, wrote a test that creates empty blk000xx.dat and rev000xx.dat files from 0 to 28, with contiguous and non-contiguous segments, and runs the CleanupBlockRevFiles() function. then, this test will be run with clang's address, memory and undefined sanitizers, in turn.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- for-range-refactor-in-init
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 8228 Build 14482: Default Diff Build & Tests Build 14481: arc lint + arc unit
Event Timeline
| src/init.cpp | ||
|---|---|---|
| 1100 ↗ | (On Diff #14263) | This is going to create copies. |
| src/init.cpp | ||
|---|---|---|
| 1100 ↗ | (On Diff #14263) | not 100% sure I understand. you mean copies of std::string? should fileName be a reference instead? |
Comment Actions
I fell like you are a bit overdoing it with auto here. I'm not against auto in general, but C++ is a bit trigger happy with copies and implicit conversions.
You also want to run this through UBSAN to make sure you are not keeping a reference to a temporary as part of your test plan.
| src/init.cpp | ||
|---|---|---|
| 1100 ↗ | (On Diff #14298) | Use const auto rather than auto const. |
Comment Actions
I have run the test using const auto &fileName and it did set off the -fsanitize=address giving me a stack-use-after-scope error, so I went back to by-value semantics. address,undefined and memory sanitizer reported no problems after that (except for one use-of-uninitialized-value on Boost)
about all that auto, I compiled my test in a local compiler explorer instance, side by side, using auto in one snippet and fully spelled out types on everything in the other, and the resulting assembly was exactly the same for both g++ and clang, would that be ok then?