New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bench: Represent paths with fs::path instead of std::string #24252
Conversation
cr ACK 3278e64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
After changing type single quotes are redundant as path
object is already quoted with double quotes:
Line 37 in 3ace3a1
std::cout << "Could write to file '" << filename << "'" << std::endl; |
Line 40 in 3ace3a1
std::cout << "Created '" << filename << "'" << std::endl; |
While here, is it a good chance for some fixes?
src/bench/bench.cpp
Outdated
@@ -24,13 +24,13 @@ const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{}; | |||
|
|||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/filename/file/ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/filename/file/ ?
Sure, changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3278e64
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 <32963518+hebasto@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/bench/bench.cpp
Outdated
@@ -24,13 +24,13 @@ const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{}; | |||
|
|||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/filename/file/ ?
Sure, changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
untested ACK 824e1ff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 824e1ff, tested on Linux Mint 20.2 (x86_64).
…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
Suggested #20744 (comment)