Get rid of VARINT default argument #18087

sipa commented at 4:05 AM on February 7, 2020: member

This removes the need for the non-strandard use of variadic macros.

fanquake added the label Refactoring on Feb 7, 2020

fanquake commented at 4:10 AM on February 7, 2020: member

Thanks. Concept ACK.

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.

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?

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.

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.

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.

fanquake requested review from ryanofsky on Feb 7, 2020

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.

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"

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

practicalswift commented at 3:31 PM on February 10, 2020: contributor

ACK c2dc9811a05f86534bd814b533271fac6b7ac68e -- patch looks correct

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:

#define VARINT(obj)            VARINT_MODE(obj, VarIntMode::DEFAULT)
#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.

ryanofsky approved

ryanofsky commented at 3:41 PM on February 10, 2020: member

Code review ACK c2dc9811a05f86534bd814b533271fac6b7ac68e. Verified changes with

git checkout origin/pull/18087/head
git grep -l VARINT | xargs sed -i 's/VARINT(\([^,]\+\), VarIntMode::DEFAULT)/VARINT(\1\)/'
git diff HEAD^

DrahtBot added the label Needs rebase on Feb 10, 2020

sipa force-pushed on Feb 10, 2020

sipa commented at 6:04 PM on February 10, 2020: member

Rebased, and switched to @ryanofsky's VARINT/VARINT_MODE approach (smaller diff!).

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.

DrahtBot removed the label Needs rebase on Feb 10, 2020

ryanofsky approved

ryanofsky commented at 6:24 PM on February 10, 2020: member

Code review ACK ace3c7157e4038c96e94f8b66490ba62439967d4. Changes since last review: rebase and adding VARINT_MODE

Get rid of VARINT default argument

This removes the need for the GNU C++ extension of variadic macros.

0e0fa27acb

sipa force-pushed on Feb 10, 2020

ryanofsky approved

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

MarcoFalke commented at 8:27 PM on February 10, 2020: member

ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb 📯

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash fe1f05c2963a24a1253bb29b2e921091390831e2f2b6c031b2109dc1573157b6 -

</details>

jonatack commented at 9:51 PM on February 10, 2020: member

ACK 0e0fa27 code review, built/ran tests/bitcoind

practicalswift commented at 6:39 AM on February 11, 2020: contributor

ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb -- diff looks correct

fanquake referenced this in commit 35b7a8e539 on Feb 11, 2020

fanquake merged this on Feb 11, 2020

fanquake closed this on Feb 11, 2020

fanquake referenced this in commit e727c2bdca on May 4, 2020

sidhujag referenced this in commit fae55b66f9 on May 5, 2020

Fabcien referenced this in commit 3a50002266 on Jan 2, 2021

furszy referenced this in commit 5c93f159bc on Jul 5, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

DrahtBot locked this on Feb 15, 2022