refactor: Print verbose serialize compiler error messages #29056

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:

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

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

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

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

sedited 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.

sedited approved

sedited commented at 2:32 PM on December 12, 2023: contributor

concept ACK fa180c523a173550ab4e549788cb21d4e7994118 :tada:

This:

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

is definitely more understandable than

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

maflcko force-pushed on Dec 12, 2023

fanquake requested review from ryanofsky on Dec 12, 2023

sedited approved

sedited 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:

BreakBeforeConceptDeclarations: Always
RequiresExpressionIndentation: 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:

template <class Stream>
concept WritableStream = requires(Stream& s, const Span<std::byte> src) { s.write(src); };

template <class T, class Stream>
concept Serializable = WritableStream<Stream> && requires(T a, Stream s) { a.Serialize(s); };

template <class Stream>
concept ReadableStream = requires(Stream& s, Span<std::byte> dst) { s.read(dst); };

template <class T, class Stream>
concept 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 sedited on Dec 19, 2023

sedited approved

sedited 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

DashCoreAutoGuix referenced this in commit 1bdd349d25 on Jul 30, 2025

DashCoreAutoGuix referenced this in commit dcf0581ec5 on Aug 7, 2025

delta1 referenced this in commit ca69c03e62 on Nov 17, 2025

Fabcien referenced this in commit d163bbd74b on Dec 4, 2025

vijaydasmp referenced this in commit 499a05d83f on Feb 2, 2026

vijaydasmp referenced this in commit 351653d9d8 on Feb 3, 2026

vijaydasmp referenced this in commit b66d2b5484 on Feb 4, 2026

vijaydasmp referenced this in commit fb56b0e57e on Feb 4, 2026

bitcoin locked this on Feb 9, 2026