Get rid of VARINT default argument #18087

pull sipa wants to merge 1 commits into bitcoin:master from sipa:202002_novarintvarmacro changing 6 files +22 −21
  1. sipa commented at 4:05 am on February 7, 2020: member
    This removes the need for the non-strandard use of variadic macros.
  2. fanquake added the label Refactoring on Feb 7, 2020
  3. fanquake commented at 4:10 am on February 7, 2020: member
    Thanks. Concept ACK.
  4. DrahtBot commented at 9:43 am on February 7, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18112 (Serialization improvements step 5 (blockencodings) by sipa)
    • #18000 ([WIP] Coin Statistics Index by fjahr)
    • #17060 (Cache 26% more coins: Reduce CCoinsMap::value_type from 96 to 76 bytes by martinus)

    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.

  5. MarcoFalke commented at 2:06 pm on February 7, 2020: member
    What is the risk or downside of using GNU extensions? Is this a stylistic issue?
  6. Empact commented at 3:28 pm on February 7, 2020: member

    Given the two current modes are:

    • DEFAULT -> unsigned
    • NONNEGATIVE_SIGNED -> signed

    Assuming other modes aren’t planned. How about using two separate macros to present them: VARINT and VARINT_U (or similar)?

    Incidentally, I don’t see anywhere we’re currently checking that the NONNEGATIVE_SIGNED values are in fact nonnegative.

  7. sipa commented at 5:34 pm on February 7, 2020: member

    What is the risk or downside of using GNU extensions? Is this a stylistic issue?

    Ping @fanquake, @theuni.

    Assuming other modes aren’t planned. How about using two separate macros to present them: VARINT and VARINT_U (or similar)? @ryanofsky What do you think of this suggestion?

    Incidentally, I don’t see anywhere we’re currently checking that the NONNEGATIVE_SIGNED values are in fact nonnegative.

    Good point. They’re only used in local disk formats, but perhaps an assert makes sense.

  8. fanquake commented at 11:13 pm on February 7, 2020: member

    Is this a stylistic issue? @MarcoFalke No. The better place for this discussion would be #18088.

  9. fanquake requested review from ryanofsky on Feb 7, 2020
  10. laanwj commented at 11:27 pm on February 7, 2020: member

    What is the risk or downside of using GNU extensions? Is this a stylistic issue?

    It means that a compiler “merely” implementing the C++11 standard could not compile our code.

    Maybe all current C++ compilers implement these extensions at the moment, but how would we feel if this would be some proprietary MSVC extension used?

    IMO, unless there is another strong reason that gives a clear advantage (say, in efficiency, or correctness/verify ability), it’s preferable to not use special extensions.

  11. MarcoFalke commented at 4:21 pm on February 9, 2020: member

    Concept ACK per @laanwj

    Slightly prefer the version by @Empact because it feels odd to explicitly pass in a value that is just called “default”

  12. practicalswift commented at 3:27 pm on February 10, 2020: contributor

    Concept ACK: inside the standard is obviously better than outside the standard :)

    Talking about standards: people interested in this PR might be interested in reviewing #17208 (“Make all tests pass UBSan without using any UBSan suppressions”) which needs review :)

  13. practicalswift commented at 3:31 pm on February 10, 2020: contributor
    ACK c2dc9811a05f86534bd814b533271fac6b7ac68e – patch looks correct
  14. in src/serialize.h:493 in c2dc9811a0 outdated
    489@@ -490,12 +490,12 @@ class Wrapper
    490 template<typename Formatter, typename T>
    491 static inline Wrapper<Formatter, T&> Using(T&& t) { return Wrapper<Formatter, T&>(t); }
    492 
    493-#define VARINT(obj, ...) Using<VarIntFormatter<__VA_ARGS__>>(obj)
    494+#define VARINT(obj, mode) Using<VarIntFormatter<mode>>(obj)
    


    ryanofsky commented at 3:33 pm on February 10, 2020:

    Would suggest something like:

    0#define VARINT(obj)            VARINT_MODE(obj, VarIntMode::DEFAULT)
    1#define VARINT_MODE(obj, mode) Using<VarIntFormatter<mode>>(obj)
    

    to reduce verbosity and number of changes needed here


    sipa commented at 6:04 pm on February 10, 2020:
    Done.
  15. ryanofsky approved
  16. ryanofsky commented at 3:41 pm on February 10, 2020: member

    Code review ACK c2dc9811a05f86534bd814b533271fac6b7ac68e. Verified changes with

    0git checkout origin/pull/18087/head
    1git grep -l VARINT | xargs sed -i 's/VARINT(\([^,]\+\), VarIntMode::DEFAULT)/VARINT(\1\)/'
    2git diff HEAD^
    
  17. DrahtBot added the label Needs rebase on Feb 10, 2020
  18. sipa force-pushed on Feb 10, 2020
  19. sipa commented at 6:04 pm on February 10, 2020: member
    Rebased, and switched to @ryanofsky’s VARINT/VARINT_MODE approach (smaller diff!).
  20. in src/serialize.h:214 in ace3c7157e outdated
    210@@ -211,7 +211,7 @@ template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
    211  *
    212  * Example use:
    213  *   struct FooFormatter {
    214- *     FORMATTER_METHODS(Class, obj) { READWRITE(obj.val1, VARINT(obj.val2)); }
    215+ *     FORMATTER_METHODS(Class, obj) { READWRITE(obj.val1, VARINT(obj.val2, VarIntMode::DEFAULT)); }
    


    MarcoFalke commented at 6:14 pm on February 10, 2020:
    :eyes: (edit: This hunk should be removed from the diff)

    sipa commented at 8:00 pm on February 10, 2020:

    Aha, someone caught it!

    (clearly I left that in to see if reviewers were paying attention)


    sipa commented at 8:00 pm on February 10, 2020:
    Fixed.
  21. DrahtBot removed the label Needs rebase on Feb 10, 2020
  22. ryanofsky approved
  23. ryanofsky commented at 6:24 pm on February 10, 2020: member
    Code review ACK ace3c7157e4038c96e94f8b66490ba62439967d4. Changes since last review: rebase and adding VARINT_MODE
  24. Get rid of VARINT default argument
    This removes the need for the GNU C++ extension of variadic macros.
    0e0fa27acb
  25. sipa force-pushed on Feb 10, 2020
  26. ryanofsky approved
  27. ryanofsky commented at 8:10 pm on February 10, 2020: member
    Code review ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb. Only change since last review reverting outdated documentation change from earlier version of pr
  28. MarcoFalke commented at 8:27 pm on February 10, 2020: member

    ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb 📯

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb 📯
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUgPJwv+JEf3TlslUjdcSz58oj7t8GSnSnNeIwcAh6zSPMC2xxFNDX7ISNolSOnM
     8Xw1ZFjx7XKxNsc1c/ZfCumK51H59fAXrDPGnXWST50W6lUMK7Ithr/AYtiviTbUL
     9xNeONWmAtjI4VjFRMtxyDJNJdkRdX7ex7P50av79lQ/DwKvIVcW4k4FPKHO183qr
    10FJKKPve6kCceMdufWaFHbHqTIUa1CtXOR1w3ZAfLH2X1SDxbSVTXwiJ3UlFwtpaI
    11jWF8Es9Jmhm3WEFTso+6Rzj61t0MNZa50CAdTV+gk/wptjgrG07ryqaHtCZLWJjf
    12LdZn0ZHiM5r0F+ArVzTGrtoqPZ2nWwRTaH5VxbK5UwfAe33woO2IFRdldlyJSaGN
    13cdlBEVYIZNxbgDehwJFytsnoDD+kGDb2bF9e202oJ6DQGP7uhb2vU8aUn7CYPa+X
    14gtA7UC41YvwVlFfURrOx7LYAFO6E/LChs3sJaZBdyhlrcETLTZxAhEphEHWqdMCZ
    15ecC5uhfq
    16=sVpp
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash fe1f05c2963a24a1253bb29b2e921091390831e2f2b6c031b2109dc1573157b6 -

  29. jonatack commented at 9:51 pm on February 10, 2020: member
    ACK 0e0fa27 code review, built/ran tests/bitcoind
  30. practicalswift commented at 6:39 am on February 11, 2020: contributor
    ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb – diff looks correct
  31. fanquake referenced this in commit 35b7a8e539 on Feb 11, 2020
  32. fanquake merged this on Feb 11, 2020
  33. fanquake closed this on Feb 11, 2020

  34. fanquake referenced this in commit e727c2bdca on May 4, 2020
  35. sidhujag referenced this in commit fae55b66f9 on May 5, 2020
  36. Fabcien referenced this in commit 3a50002266 on Jan 2, 2021
  37. furszy referenced this in commit 5c93f159bc on Jul 5, 2021
  38. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  39. DrahtBot locked this on Feb 15, 2022

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-12-22 21:12 UTC

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