make ParseOutputType return a std::optional<OutputType> #22621

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:parse_output_type_optional changing 7 files +40 −36

fanquake commented at 5:40 AM on August 4, 2021: member

Similar to #22220. Skipped using auto here for the same reasons outlined in that PR.
practicalswift commented at 5:52 AM on August 4, 2021: contributor

Concept ACK
DrahtBot added the label RPC/REST/ZMQ on Aug 4, 2021
DrahtBot added the label Wallet on Aug 4, 2021

in src/test/fuzz/kitchen_sink.cpp:52 in fb52c43a64 outdated

  51 | -    const bool parsed = ParseOutputType(output_type_string, output_type_parsed);
  52 | -    assert(parsed);
  53 | -    assert(output_type == output_type_parsed);
  54 | -    (void)ParseOutputType(fuzzed_data_provider.ConsumeRandomLengthString(64), output_type_parsed);
  55 | +    const std::optional<OutputType> parsed = ParseOutputType(output_type_string);
  56 | +    assert(output_type == parsed.value());

jnewbery commented at 10:46 AM on August 4, 2021:

We should assert that parsed has a value before extracting the object:

    assert(parsed);
    assert(output_type == parsed.value());

fanquake commented at 11:13 AM on August 4, 2021:

This just round-trips known values, so I don't think it can ever not have a value. However we can re-add the assert.

jnewbery commented at 10:47 AM on August 4, 2021: member

utACK fb52c43a6461c5c4fee47d7442703c48d7b91c15
make ParseOutputType return a std::optional<OutputType> 32fa49a184
in src/outputtype.h:32 in fb52c43a64 outdated
```
  28 | @@ -28,7 +29,7 @@ static constexpr auto OUTPUT_TYPES = std::array{
  29 |      OutputType::BECH32M,
  30 |  };
  31 |  
  32 | -[[nodiscard]] bool ParseOutputType(const std::string& str, OutputType& output_type);
  33 | +std::optional<OutputType> ParseOutputType(const std::string& str);
```
jnewbery commented at 10:53 AM on August 4, 2021:

Is there a reason for removing the [[nodiscard]] attribute?

fanquake commented at 11:12 AM on August 4, 2021:

I'm not sure why we'd keep it now that we're returning the result. If it's unused the compiler should emit Wunused-result ?

jnewbery commented at 12:15 PM on August 4, 2021:

Makes sense
fanquake force-pushed on Aug 4, 2021
jnewbery commented at 12:16 PM on August 4, 2021: member

utACK 32fa49a18497a9b8c72e36a72ae96e7b23930223
jonatack commented at 12:44 PM on August 4, 2021: member

Code review ACK 32fa49a18497a9b8c72e36a72ae96e7b23930223 and debian clang 13 debug build is clean / unit tests locally are green

nit, can ignore: parsed localvars could be declared const, e.g. as in src/test/fuzz/kitchen_sink.cpp

MarcoFalke commented at 5:01 PM on August 4, 2021: member

review ACK 32fa49a18497a9b8c72e36a72ae96e7b23930223 🍢

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 91c943e8fe3b8d3fba23d0d084119eabd4fe46e1ab28f6b682bc8aa124a49d61 -

</details>

MarcoFalke merged this on Aug 4, 2021
MarcoFalke closed this on Aug 4, 2021
sidhujag referenced this in commit 7f55ebaabc on Aug 4, 2021
fanquake deleted the branch on Aug 5, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#22220 util: make ParseMoney return a std::optional<CAmount>