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-
sipa commented at 4:05 am on February 7, 2020: memberThis 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: memberThanks. 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: memberWhat 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
andVARINT_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?
Assuming other modes aren’t planned. How about using two separate macros to present them:
VARINT
andVARINT_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
-
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: contributorACK 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:
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. -
ryanofsky approved
-
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^
-
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: memberRebased, 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: memberCode 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.
-
sipa force-pushed on Feb 10, 2020
-
ryanofsky approved
-
ryanofsky commented at 8:10 pm on February 10, 2020: memberCode 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 📯
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 -
-
jonatack commented at 9:51 pm on February 10, 2020: memberACK 0e0fa27 code review, built/ran tests/bitcoind
-
practicalswift commented at 6:39 am on February 11, 2020: contributorACK 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