refactor: Print verbose serialize compiler error messages

maflcko commented at 9:21 pm on December 11, 2023: member

Currently, trying to serialize an object that can’t be serialized will fail with a short error message. For example, the diff and the error message:

 0diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
 1index d75eb499b4..773f49845b 100644
 2--- a/src/test/serialize_tests.cpp
 3+++ b/src/test/serialize_tests.cpp
 4@@ -62,6 +62,8 @@ public:
 5 
 6 BOOST_AUTO_TEST_CASE(sizes)
 7 {
 8+    int b[4];
 9+    DataStream{} << b << Span{b};
10     BOOST_CHECK_EQUAL(sizeof(unsigned char), GetSerializeSize((unsigned char)0));
11     BOOST_CHECK_EQUAL(sizeof(int8_t), GetSerializeSize(int8_t(0)));
12     BOOST_CHECK_EQUAL(sizeof(uint8_t), GetSerializeSize(uint8_t(0)));

0./serialize.h:765:6: error: member reference base type 'const int[4]' is not a structure or union
1  765 |     a.Serialize(os);
2      |     ~^~~~~~~~~~

0./serialize.h:277:109: error: no matching function for call to 'UCharCast'
1  277 | template <typename Stream, typename B> void Serialize(Stream& s, Span<B> span) { (void)/* force byte-type */UCharCast(span.data()); s.write(AsBytes(span)); }
2      |                                                                                                             ^~~~~~~~~

This is fine. However, it would be more helpful for developers and more accurate by the compiler to explain why each function is not selected.

Fix this by using C++20 concepts where appropriate.

DrahtBot commented at 9:21 pm on December 11, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, ajtowns, achow101
Concept ACK	ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot renamed this:
~~refactor: Print verbose serialize compiler error messages~~
refactor: Print verbose serialize compiler error messages
on Dec 11, 2023

DrahtBot added the label Refactoring on Dec 11, 2023

maflcko force-pushed on Dec 11, 2023

in src/span.h:294 in fad89dadd2 outdated

290@@ -290,6 +291,7 @@ inline unsigned char* UCharCast(std::byte* c) { return reinterpret_cast<unsigned
291 inline const unsigned char* UCharCast(const char* c) { return reinterpret_cast<const unsigned char*>(c); }
292 inline const unsigned char* UCharCast(const unsigned char* c) { return c; }
293 inline const unsigned char* UCharCast(const std::byte* c) { return reinterpret_cast<const unsigned char*>(c); }
294+static_assert(std::is_same_v<unsigned char, uint8_t>); // This codebase assumes that uint8_t exists and is equal to unsigned char

TheCharlatan commented at 1:22 pm on December 12, 2023:

Nit: Should this rather be in assumptions.h if it counts for the entire codebase?

maflcko commented at 2:45 pm on December 12, 2023:

Yes, but I guess it is not too useful, so I’ve dropped the commit for now.

TheCharlatan approved

TheCharlatan commented at 2:32 pm on December 12, 2023: contributor

concept ACK fa180c523a173550ab4e549788cb21d4e7994118 :tada:

This:

0./serialize.h:763:6: note: candidate template ignored: constraints not satisfied [with Stream = DataStream, T = Span<int>]
1void Serialize(Stream& os, const T& a)
2     ^
3./serialize.h:762:14: note: because 'Serializable<Span<int>, DataStream>' evaluated to false
4    requires Serializable<T, Stream>
5             ^
6./serialize.h:760:52: note: because 'a.Serialize(s)' would be invalid: no member named 'Serialize' in 'Span<int>'
7concept Serializable = requires(T a, Stream s) { a.Serialize(s); };
8                                                   ^
91 error generated.

is definitely more understandable than

0./serialize.h:765:6: error: member reference base type 'const int[4]' is not a structure or union
1    a.Serialize(os);
2    ~^~~~~~~~~~

maflcko force-pushed on Dec 12, 2023

fanquake requested review from ryanofsky on Dec 12, 2023

TheCharlatan approved

TheCharlatan commented at 3:49 pm on December 12, 2023: contributor

Re-ACK fa45567e030272d34845e6b563a6b626b9eda739

ismaelsadeeq commented at 12:00 pm on December 14, 2023: member

Concept ACK

in src/span.h:296 in fa45567e03 outdated

292 inline const unsigned char* UCharCast(const char* c) { return reinterpret_cast<const unsigned char*>(c); }
293 inline const unsigned char* UCharCast(const unsigned char* c) { return c; }
294 inline const unsigned char* UCharCast(const std::byte* c) { return reinterpret_cast<const unsigned char*>(c); }
295+// Helper concept for the basic byte type.
296+template <typename B>
297+concept BasicByte = requires { UCharCast(std::span<B>{}.data()); };

ajtowns commented at 1:53 am on December 15, 2023:

Perhaps describe it as “Helper concept for basic byte types” ? “BasicByte” is a concept that applies to types, not a type itself? Could name the concept as BasicByteType since it applies to types?

Consider adding .clang-format fields:

0BreakBeforeConceptDeclarations: Always
1RequiresExpressionIndentation: OuterScope

maflcko commented at 2:20 pm on December 15, 2023:

Thanks,

added “s”
Left it as BasicByte for now, to mirror the stdlib naming: https://en.cppreference.com/w/cpp/concepts/floating_point
Added clang-format fields

in src/serialize.h:760 in fa45567e03 outdated

758  * If none of the specialized versions above matched, default to calling member function.
759  */
760-template<typename Stream, typename T>
761-inline void Serialize(Stream& os, const T& a)
762+template <class T, class Stream>
763+concept Serializable = requires(T a, Stream s) { a.Serialize(s); };

ajtowns commented at 2:18 am on December 15, 2023:

Could consider:

 0template <class Stream>
 1concept WritableStream = requires(Stream& s, const Span<std::byte> src) { s.write(src); };
 2
 3template <class T, class Stream>
 4concept Serializable = WritableStream<Stream> && requires(T a, Stream s) { a.Serialize(s); };
 5
 6template <class Stream>
 7concept ReadableStream = requires(Stream& s, Span<std::byte> dst) { s.read(dst); };
 8
 9template <class T, class Stream>
10concept Unserializable = ReadableStream<Stream> && requires(T a, Stream s) { a.Unserialize(s); };

maflcko commented at 2:20 pm on December 15, 2023:

Shouldn’t WritableStream be applied at all implementations instead to constrain them? Otherwise, one could still call Object{}.Serialize(WrongStream{}) in other parts of the code?

ajtowns commented at 3:26 pm on December 15, 2023:

Yeah, that’s probably reasonable

ajtowns commented at 4:23 am on December 15, 2023: contributor

utACK fa45567e030272d34845e6b563a6b626b9eda739

DrahtBot requested review from ismaelsadeeq on Dec 15, 2023

refactor: Print verbose serialize compiler error messages fa898e6836

Allow std::byte C-style array serialization fae526345d

maflcko force-pushed on Dec 15, 2023

maflcko requested review from TheCharlatan on Dec 19, 2023

TheCharlatan approved

TheCharlatan commented at 3:55 pm on December 19, 2023: contributor

Re-ACK fae526345de539ab8f9b80100f6dfbe8e1d3284b

DrahtBot requested review from ajtowns on Dec 19, 2023

ajtowns commented at 5:48 am on December 20, 2023: contributor

reACK fae526345de539ab8f9b80100f6dfbe8e1d3284b

DrahtBot removed review request from ajtowns on Dec 20, 2023

achow101 commented at 5:08 pm on December 21, 2023: member

ACK fae526345de539ab8f9b80100f6dfbe8e1d3284b

achow101 merged this on Dec 21, 2023

achow101 closed this on Dec 21, 2023

maflcko deleted the branch on Dec 22, 2023

calmbeforeu approved

refactor: Print verbose serialize compiler error messages #29056

Code Coverage

Reviews