bench: Represent paths with fs::path instead of std::string#24252
Merged
fanquake merged 1 commit intobitcoin:masterfrom Feb 5, 2022
Merged
bench: Represent paths with fs::path instead of std::string#24252fanquake merged 1 commit intobitcoin:masterfrom
fanquake merged 1 commit intobitcoin:masterfrom
Conversation
Member
|
cr ACK 3278e64 |
hebasto
reviewed
Feb 3, 2022
Member
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
Concept ACK.
After changing type single quotes are redundant as path object is already quoted with double quotes:
Line 37 in 3ace3a1
Line 40 in 3ace3a1
While here, is it a good chance for some fixes?
src/bench/bench.cpp
Outdated
| namespace { | ||
|
|
||
| void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const std::string& filename, const char* tpl) | ||
| void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const fs::path& filename, const char* tpl) |
Contributor
Author
There was a problem hiding this comment.
nit: s/filename/file/ ?
Sure, changed
fanquake
approved these changes
Feb 4, 2022
Also uses fs::path quoting in bench printed strings and fixes a misleading error message. Originally suggested bitcoin#20744 (comment) Co-authored-by: Hennadii Stepanov <[email protected]>
ryanofsky
commented
Feb 4, 2022
src/bench/bench.cpp
Outdated
| namespace { | ||
|
|
||
| void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const std::string& filename, const char* tpl) | ||
| void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const fs::path& filename, const char* tpl) |
Contributor
Author
There was a problem hiding this comment.
nit: s/filename/file/ ?
Sure, changed
fanquake
approved these changes
Feb 4, 2022
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 6, 2022
…std::string 824e1ff bench: Represents paths with fs::path instead of std::string (Ryan Ofsky) Pull request description: Suggested bitcoin#20744 (comment) ACKs for top commit: fanquake: untested ACK 824e1ff hebasto: ACK 824e1ff, tested on Linux Mint 20.2 (x86_64). Tree-SHA512: 348fc189f30b5ad9a8e49e95e535d2c044462a9d534c3f34d887fbde0c05c41e88e02b4fc340709e6395a1188496a8333eb9e734310aa4c41755ec080e53c06e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggested #20744 (comment)