bench: Represent paths with fs::path instead of std::string #24252

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/bp changing 3 files +11 −10

ryanofsky commented at 3:55 PM on February 3, 2022: member

Suggested #20744 (comment)
DrahtBot added the label Tests on Feb 3, 2022
MarcoFalke commented at 4:05 PM on February 3, 2022: member

cr ACK 3278e64a22150e6941818deebd1622b715c0f0be

in src/bench/bench.cpp:27 in 3278e64a22 outdated

  23 | @@ -24,13 +24,13 @@ const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
  24 |  
  25 |  namespace {
  26 |  
  27 | -void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const std::string& filename, const char* tpl)
  28 | +void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const fs::path& filename, const char* tpl)

hebasto commented at 5:46 PM on February 3, 2022:

nit: s/filename/file/ ?

ryanofsky commented at 2:32 PM on February 4, 2022:

nit: s/filename/file/ ?

Sure, changed

hebasto commented at 5:54 PM on February 3, 2022: member
Concept ACK.

After changing type single quotes are redundant as path object is already quoted with double quotes: https://github.com/bitcoin/bitcoin/blob/3ace3a17c9bce606cea05192f0da3ac62ac69dda/src/bench/bench.cpp#L37 https://github.com/bitcoin/bitcoin/blob/3ace3a17c9bce606cea05192f0da3ac62ac69dda/src/bench/bench.cpp#L40

While here, is it a good chance for some fixes?
1. It seems a negation should be used in the following message: https://github.com/bitcoin/bitcoin/blob/3ace3a17c9bce606cea05192f0da3ac62ac69dda/src/bench/bench.cpp#L37
2. Shouldn't the following line https://github.com/bitcoin/bitcoin/blob/3ace3a17c9bce606cea05192f0da3ac62ac69dda/src/bench/bench.cpp#L40 be moved into the if branch above?
fanquake approved
fanquake commented at 1:29 AM on February 4, 2022: member

ACK 3278e64a22150e6941818deebd1622b715c0f0be

bench: Represents paths with fs::path instead of std::string

Also uses fs::path quoting in bench printed strings and fixes a
misleading error message.

Originally suggested https://github.com/bitcoin/bitcoin/pull/20744#issuecomment-1022486215

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

824e1ffa9f

ryanofsky force-pushed on Feb 4, 2022
ryanofsky commented at 2:35 PM on February 4, 2022: member

Updated 3278e64a22150e6941818deebd1622b715c0f0be -> 824e1ffa9fd957d05e34f36abe381c2465d89702 (pr/bp.1 -> pr/bp.2, compare) with suggested changes. Thanks!
fanquake approved
fanquake commented at 3:14 PM on February 4, 2022: member

untested ACK 824e1ffa9fd957d05e34f36abe381c2465d89702
hebasto approved
hebasto commented at 6:35 PM on February 4, 2022: member

ACK 824e1ffa9fd957d05e34f36abe381c2465d89702, tested on Linux Mint 20.2 (x86_64).
fanquake merged this on Feb 5, 2022
fanquake closed this on Feb 5, 2022
sidhujag referenced this in commit 6c7e78fb94 on Feb 6, 2022
DrahtBot locked this on Feb 5, 2023

Contributors

Labels

Linked (view graph)

#20744 Use std::filesystem. Remove Boost Filesystem & System