refactor: Print verbose serialize compiler error messages #29056

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2312-ser-err-msg- changing 3 files +23 −14
  1. 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.

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

  3. DrahtBot renamed this:
    refactor: Print verbose serialize compiler error messages
    refactor: Print verbose serialize compiler error messages
    on Dec 11, 2023
  4. DrahtBot added the label Refactoring on Dec 11, 2023
  5. maflcko force-pushed on Dec 11, 2023
  6. 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.
  7. TheCharlatan approved
  8. 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    ~^~~~~~~~~~
    
  9. maflcko force-pushed on Dec 12, 2023
  10. fanquake requested review from ryanofsky on Dec 12, 2023
  11. TheCharlatan approved
  12. TheCharlatan commented at 3:49 pm on December 12, 2023: contributor
    Re-ACK fa45567e030272d34845e6b563a6b626b9eda739
  13. ismaelsadeeq commented at 12:00 pm on December 14, 2023: member
    Concept ACK
  14. 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,

  15. 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
  16. ajtowns commented at 4:23 am on December 15, 2023: contributor
    utACK fa45567e030272d34845e6b563a6b626b9eda739
  17. DrahtBot requested review from ismaelsadeeq on Dec 15, 2023
  18. refactor: Print verbose serialize compiler error messages fa898e6836
  19. Allow std::byte C-style array serialization fae526345d
  20. maflcko force-pushed on Dec 15, 2023
  21. maflcko requested review from TheCharlatan on Dec 19, 2023
  22. TheCharlatan approved
  23. TheCharlatan commented at 3:55 pm on December 19, 2023: contributor
    Re-ACK fae526345de539ab8f9b80100f6dfbe8e1d3284b
  24. DrahtBot requested review from ajtowns on Dec 19, 2023
  25. ajtowns commented at 5:48 am on December 20, 2023: contributor
    reACK fae526345de539ab8f9b80100f6dfbe8e1d3284b
  26. DrahtBot removed review request from ajtowns on Dec 20, 2023
  27. achow101 commented at 5:08 pm on December 21, 2023: member
    ACK fae526345de539ab8f9b80100f6dfbe8e1d3284b
  28. achow101 merged this on Dec 21, 2023
  29. achow101 closed this on Dec 21, 2023

  30. maflcko deleted the branch on Dec 22, 2023
  31. calmbeforeu approved

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me