refactor: Use deduction guide for std::array<uint8_t, N> #20571

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:201204-u8 changing 3 files +42 −26

hebasto commented at 8:46 PM on December 4, 2020: member

Using the C++11 std::array with explicit template parameters is problematic because overshooting the size will fill the memory with default constructed types.

Credits to MarcoFalke.
DrahtBot added the label P2P on Dec 4, 2020
DrahtBot added the label Refactoring on Dec 4, 2020
DrahtBot added the label Utils/log/libs on Dec 4, 2020
practicalswift commented at 11:39 PM on December 4, 2020: contributor

Concept ACK
MarcoFalke removed the label P2P on Dec 5, 2020
MarcoFalke removed the label Utils/log/libs on Dec 5, 2020
in src/util/string.h:93 in 1f16514614 outdated
```
  86 | @@ -87,4 +87,9 @@ template <typename T1, size_t PREFIX_LEN>
  87 |             std::equal(std::begin(prefix), std::end(prefix), std::begin(obj));
  88 |  }
  89 |  
  90 | +/**
  91 | + * User-defined literal for byte constants.
  92 | + */
  93 | +constexpr uint8_t operator ""_u8(unsigned long long byte) { return byte; }
```
elichai commented at 10:14 AM on December 6, 2020:

Maybe we should add some overflow assert here? it would be nice if 256_u8 won't compile. the downside is that this function might be used outside of constexpr, but that's very unlikely

elichai commented at 10:14 AM on December 6, 2020:

ie assert(byte <= std::numeric_limits<uint8_t>::max());

hebasto commented at 1:46 PM on December 6, 2020:

Maybe we should add some overflow assert here? it would be nice if 256_u8 won't compile.

Agree...

ie assert(byte <= std::numeric_limits<uint8_t>::max());

... but this won't work, unfortunately.

elichai commented at 2:17 PM on December 6, 2020:

Why not? https://godbolt.org/z/hTjch7

hebasto commented at 2:20 PM on December 6, 2020:

It is not a compile-time error.

elichai commented at 4:02 PM on December 6, 2020:

You're right, idk how I didn't realized that

sipa commented at 6:33 PM on December 6, 2020:

You can use runtime asserts in constexpr functions. It just means invocations that would trigger that revert to being runtime evaluated. If the result is assigned to a constexpr variable, it'll error at compile time.

MarcoFalke commented at 6:52 PM on December 6, 2020:

Why does c++ not allow constexpr params, so that a static_assert could be used here?

hebasto commented at 6:53 PM on December 6, 2020:

:man_shrugging:

sipa commented at 7:12 PM on December 6, 2020:

C++20 adds consteval which would be useful here.

MarcoFalke commented at 7:23 PM on December 6, 2020:

I'd probably prefer std::make_array to be backported. https://en.cppreference.com/w/cpp/experimental/make_array

This would allow to write MakeArray<uint8_t>(1,2,3)

hebasto commented at 7:53 PM on December 6, 2020:

std::to_array from C++20 ?
hebasto marked this as a draft on Dec 6, 2020
refactor: Use deduction guide for std::array<uint8_t, N> b6fb4fa999
hebasto marked this as ready for review on Dec 6, 2020
hebasto force-pushed on Dec 6, 2020
hebasto commented at 3:25 PM on December 6, 2020: member
Updated 1f16514614d37edd6e916e0aca731d445101f8f7 -> b6fb4fa999b927dce07bce239532b0f07bd393a2 (pr20571.01 -> pr20571.02, diff):
- addressed @elichai's comment:
Maybe we should add some overflow assert here? it would be nice if 256_u8 won't compile.

To make compiler proceed checking, all of the std::array definitions must be specified with constexpr.
hebasto commented at 9:03 PM on December 6, 2020: member

Closing as this solution seems not future-proof.
hebasto closed this on Dec 6, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#20566 refactor: Use C++17 std::array where possible