serialization: replace char-is-int8_t autoconf detection with c++20 concept #29484

1. 
   theuni commented at 8:05 PM on February 26, 2024: member

   Doesn't depend on #29263, but it's really only relevant after that one's merged.

   This removes the only remaining autoconf macro in our serialization code (after #29263), so it can now be used trivially and safely out-of-tree.

   ~Our code does not currently contain any concepts, but couldn't find any discussion or docs about avoiding them. I guess we'll see if this blows up our c-i.~ Edit: Ignore this. ajtowns pointed out that we're already using a few concepts.

   This was introduced in #13580. Please check my logic on this as I'm unable to test on a SmartOS system. Even better would be a confirmation from someone who can build there.

2. 
   DrahtBot commented at 8:05 PM on February 26, 2024: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Empact
   Concept ACK	fanquake
   Stale ACK	maflcko, TheCharlatan

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #29494 (build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes by maflcko)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. 
   theuni commented at 8:07 PM on February 26, 2024: member

   Ping @maflcko for the concepts question. Ping @TheCharlatan as the autoconf removal might be relevant to kernel. Ping @stacepellegrino for a possible test.

4. 
   fanquake commented at 9:24 AM on February 27, 2024: member

   Concept ACK

5. 
   maflcko commented at 4:34 PM on February 27, 2024: member

   review ACK 064c6d37d6467b2fddb9f85efd40a5dd4002d9a6

6. DrahtBot requested review from fanquake on Feb 27, 2024
7. maflcko requested review from Empact on Feb 27, 2024
8. TheCharlatan approved
9. 
   TheCharlatan commented at 4:58 PM on February 27, 2024: contributor

   ACK 064c6d37d6467b2fddb9f85efd40a5dd4002d9a6

10. in src/serialize.h:270 in 064c6d37d6 outdated

     268 | +// in terms of char. Forbid serialization of char in the typical case, but allow it if
     269 | +// it's the only way to describe an int8_t.
     270 | +template<class T>
     271 | +concept CharNotInt8 = std::same_as<T, char> && !std::same_as<T, int8_t>;
     272 | +
     273 | +template <typename Stream, CharNotInt8 V > void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
    

    ---

    ---

    

    Empact commented at 5:58 PM on February 27, 2024:

    nit: V > whitespace

11. Empact approved
12. 
    Empact commented at 5:59 PM on February 27, 2024: contributor

    Code review ACK https://github.com/bitcoin/bitcoin/commit/064c6d37d6467b2fddb9f85efd40a5dd4002d9a6

13. serialization: replace char-is-int8_t autoconf detection with c++20 concept

    This removes the only remaining autoconf macro in our serialization code,
    so it can now be used trivially and safely out-of-tree.

    ad7584d8b6
14. theuni force-pushed on Feb 27, 2024
15. DrahtBot requested review from maflcko on Feb 27, 2024
16. DrahtBot requested review from TheCharlatan on Feb 27, 2024
17. 
    Empact commented at 9:12 PM on February 27, 2024: contributor

    Code review ACK https://github.com/bitcoin/bitcoin/commit/ad7584d8b60119ca3717117a1eb6a16d753c5d74

18. fanquake merged this on Feb 28, 2024
19. fanquake closed this on Feb 28, 2024

    ---

20. hebasto referenced this in commit efd326791e on Mar 17, 2024
21. bitcoin locked this on Feb 27, 2025

Review Requested
fanquakemaflckoTheCharlatan