Starting with C++17, std::byte is often (not always) a better choice over uint8_t for new code.
However, the existing codebase discourages the use of std::byte, when helpers such as ReadLE32 are used. This is because calling code will be cluttered with byte-casts.
Fix it by allowing std::byte pointers in ReadLE32 (and friends).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31524.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Nice use of concepts.
utACK fa83bec78ef3e86445e522afa396c97b58eb1902
18+
19+template <ByteType B>
20+inline uint16_t ReadLE16(const B* ptr)
21 {
22 uint16_t x;
23 memcpy(&x, ptr, 2);
nit: given the simplicity of this PR, if you edit again, consider if it would be simpler to use sizeof expressions instead of the constants:
0 memcpy(&x, ptr, sizeof(uint16_t));
15-uint16_t static inline ReadLE16(const unsigned char* ptr)
16+template <typename B>
17+concept ByteType = std::same_as<B, unsigned char> || std::same_as<B, std::byte>;
18+
19+template <ByteType B>
20+inline uint16_t ReadLE16(const B* ptr)
as far as I can tell inline templates are redundant here (constexpr also compiles, but not sure it’s valid):
0template <ByteType B>
1uint16_t ReadLE16(const B* ptr)
constexpr, so adding it serves no purpose.
inline
I don’t have a strong feeling, see #30933 (review).
My preference would be to have a clang-tidy (plugin) rule for this, instead of using human review resources.
In commit “refactor: Allow std::byte in Read(LE/BE)” (fa83bec78ef3e86445e522afa396c97b58eb1902)
There is a shorter syntax for this that I think would make code easier to read (thoughout this file):
0-template <ByteType B>
1-inline uint16_t ReadLE16(const B* ptr)
2+inline uint16_t ReadLE16(const ByteType auto* ptr)
ByteType auto construct slightly more confusing than the more verbose template.
Personally, I find the
ByteType autoconstruct slightly more confusing than the more verbose template.
Ok, resolving for now.
If there is a preference about the style, my preference would be to have a clang-tidy (plugin) rule for this, instead of using human review resources.
I don’t think it’s necessary to change this, but the principle here is to avoid having unnecessary template functions and unnecessary template parameters when it is possible to use concepts directly. In this case there’s no point in defining a type B to stand in for ByteType and then using B one time when you can just use ByteType directly.
I don’t think it would be possible to write a clang-tidy plugin to check for these cases because the check would need to scan the codebase and ensure there were no calls to these functions that were passed explicit template arguments. Clang-tidy can’t know if if a function is intended to be used as template function, or if it is just intended to be a generic function that references a few concepts. The point of writing more direct code is to reflect this intent.
@ryanofsky I don’t think that’s right; these are all just different notations for the same thing:
void func(ByteType auto b)template<ByteType T> void func(T b)template<typename T> requires ByteType<T> void func(T b)So even the shortest version can be invoked with explicitly template arguments (func<unsigned char>(5) for example).
No opinion on whether the usage of Concept auto arguments should be encouraged, and/or the long version of it outlawed. But I do believe it’s possible to write a tidy rule for it in theory.
I don’t think it would be possible to write a clang-tidy plugin to check for these cases
I haven’t tried it, so I am not sure if it is possible. However, looking at the AST, it should be trivial to detect those functions. Matching any FunctionTemplateDecl whose FunctionDecl does not have any TemplateTypeParm in the AST should be sufficient to find those that can be written without template<...>.
Example: https://godbolt.org/z/4rre4rPT6
Thanks sipa and maflcko. I didn’t realize you were allowed to pass explicit template arguments to functions declared without template parameters, but it makes sense given the equivalence shown. It is also interesting to see ASTs of these functions in godbolt, even though I did understand that clang-tidy could see when template parameters were referenced.
Upshot seems to be that it would be possible to write a clang-tidy linter that would not force you to write broken code (because template arguments can be passed to functions not written with template parameters). But this linter could still force you to write code that does not reflect your intent, if you are are intending for a function to accept template arguments and the linter forces you to drop the parameters.
I have to say, I don’t actually understand why 3 experienced c++ developers (maflcko, l0rinc, sipa) would be on the fence in this case about whether functions not intended to accept template parameters should be written as function templates, but vive la difference.
12 #include <cstdint>
13 #include <cstring>
14
15-uint16_t static inline ReadLE16(const unsigned char* ptr)
16+template <typename B>
17+concept ByteType = std::same_as<B, unsigned char> || std::same_as<B, std::byte>;
Byte type means that size is 1, right? If you edit again, consider simplifying to something like:
0template <typename B>
1concept ByteType = (sizeof(B) == 1);
Or if you don’t like it, to the mentioned uint8_t
0template <typename B>
1concept ByteType = std::same_as<B, uint8_t> || std::same_as<B, std::byte>;
No, signed chars are explicitly disallowed. See also: git grep delete ./src/serialize.h
0src/serialize.h:template <typename Stream, CharNotInt8 V> void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
1src/serialize.h:template <typename Stream, CharNotInt8 V> void Unserialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
46+template <ByteType B>
47+inline void WriteLE16(B* ptr, uint16_t x)
48 {
49 uint16_t v = htole16_internal(x);
50 memcpy(ptr, &v, 2);
51 }
We could const the parameter and locals to obviate which one we’re writing:
0template <ByteType B>
1void WriteLE16(B* ptr, const uint16_t x)
2{
3 const uint16_t v{htole16_internal(x)};
4 memcpy(ptr, &v, 2);
5}
38+ input[2] = ReadLE32(key.data() + 8);
39+ input[3] = ReadLE32(key.data() + 12);
40+ input[4] = ReadLE32(key.data() + 16);
41+ input[5] = ReadLE32(key.data() + 20);
42+ input[6] = ReadLE32(key.data() + 24);
43+ input[7] = ReadLE32(key.data() + 28);
maybe we can roll back the loop now to something like:
0void ChaCha20Aligned::SetKey(Span<const std::byte> key) noexcept
1{
2 assert(key.size() == KEYLEN);
3 constexpr int words{KEYLEN / sizeof(uint32_t)};
4 for (int i{0}; i < words; ++i) input[i] = ReadLE32(key.data() + i * sizeof(uint32_t));
5 std::memset(&input[words], 0, (std::size(input) - words) * sizeof(uint32_t));
6}
ACK fa83bec78ef3e86445e522afa396c97b58eb1902
Please see if any of the simplification suggestions apply, but I’m fine with it as is
49 uint16_t v = htole16_internal(x);
50 memcpy(ptr, &v, 2);
51 }
52
53-void static inline WriteLE32(unsigned char* ptr, uint32_t x)
54+template <ByteType B>
The title claims that only reads were changed, but since we have also adapted the writes, would it make sense to modify them in the example ChaCha as well to demonstrate their correctness?
0diff --git a/src/crypto/chacha20.cpp b/src/crypto/chacha20.cpp
1--- a/src/crypto/chacha20.cpp (revision fa83bec78ef3e86445e522afa396c97b58eb1902)
2+++ b/src/crypto/chacha20.cpp (date 1735812552447)
3@@ -59,7 +59,7 @@
4
5 inline void ChaCha20Aligned::Keystream(Span<std::byte> output) noexcept
6 {
7- unsigned char* c = UCharCast(output.data());
8+ std::byte* c = output.data();
9 size_t blocks = output.size() / BLOCKLEN;
10 assert(blocks * BLOCKLEN == output.size());
11
12@@ -161,8 +161,8 @@
13 inline void ChaCha20Aligned::Crypt(Span<const std::byte> in_bytes, Span<std::byte> out_bytes) noexcept
14 {
15 assert(in_bytes.size() == out_bytes.size());
16- const unsigned char* m = UCharCast(in_bytes.data());
17- unsigned char* c = UCharCast(out_bytes.data());
18+ const std::byte* m = in_bytes.data();
19+ std::byte* c = out_bytes.data();
20 size_t blocks = out_bytes.size() / BLOCKLEN;
21 assert(blocks * BLOCKLEN == out_bytes.size());
