refactor: Serialization parameter cleanups

ajtowns commented at 0:30 am on September 14, 2023: contributor

Cleanups after #25284:

ser_action namespacing - #25284 (review)
make reference implicit - #25284 (review)
function notation - #25284 (comment)

serialize: move ser_action functions out of global namespace bf147bfffa

serialize: specify type for ParamsWrapper not ref 33203f59b4

serialize: add SER_PARAMS_OPFUNC 5e5c8f86b6

scripted-diff: use SER_PARAMS_OPFUNC

-BEGIN VERIFY SCRIPT-
sed -i 's/WithParams(\(CAddress::V[12]_[A-Z]*\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's/WithParams(\(CNetAddr::V[12]\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's@\(CNetAddr::V1.CService{}.*\)    //@\1                //@' src/test/util/net.cpp
-END VERIFY SCRIPT-

fb6a2ab63e

DrahtBot commented at 0:30 am on September 14, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, TheCharlatan

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28451 (Remove unused SER_DISK, SER_NETWORK, SER_GETHASH by MarcoFalke)
#26859 (fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses by vasild)

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.

in src/serialize.h:1051 in fb6a2ab63e

1049 struct ActionUnserialize {
1050-    constexpr bool ForRead() const { return true; }
1051-};
1052+    static constexpr bool ForRead() { return true; }
1053 
1054+    template<typename Stream, typename... Args>

maflcko commented at 9:38 am on September 14, 2023:

nit in bf147bfffa1afb11721f30e83eec1fa829f64d5f: Forgot to run clang-format? (Shouldn’t cause any issues, because reviewers will have to use --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space either way)

DrahtBot renamed this:
~~Serialization parameter cleanups~~
refactor: Serialization parameter cleanups
on Sep 14, 2023

DrahtBot added the label Refactoring on Sep 14, 2023

maflcko commented at 10:07 am on September 14, 2023: member

I think the last commit is incomplete? For example, the following removal compiles fine for me locally on top of this pull and master:

 0diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
 1index 036355b5f1..37b670662f 100644
 2--- a/src/test/net_tests.cpp
 3+++ b/src/test/net_tests.cpp
 4@@ -331,7 +331,7 @@ BOOST_AUTO_TEST_CASE(cnetaddr_serialize_v1)
 5     DataStream s{};
 6     const auto ser_params{CAddress::V1_NETWORK};
 7 
 8-    s << WithParams(ser_params, addr);
 9+    s << ser_params( addr);
10     BOOST_CHECK_EQUAL(HexStr(s), "00000000000000000000000000000000");
11     s.clear();
12

I think it may be good to try to nuke WithParams completely. This would also address the outstanding feedback:

#25284 (review)
and #25284 (review)

maflcko approved

maflcko commented at 10:07 am on September 14, 2023: member

lgtm ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0 💨

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0 💨
3ywac3bAMrnfFHQLulX4zEWpRkiedwedVsrfX38Q39iL2lYR4s+D+pEv/I+9r777WVdScxjp7uMSq+J08QgcZDg==

fanquake requested review from theuni on Sep 14, 2023

fanquake requested review from TheCharlatan on Sep 14, 2023

ajtowns commented at 10:33 am on September 14, 2023: contributor

I think the last commit is incomplete? For example, the following removal compiles fine for me locally on top of this pull and master:

s « WithParams(ser_params, addr);

s « ser_params( addr);

I only changed the ones that had constexpr parameters – it’s pretty clear what V1_DISK(info) means, but if you generalise it to anything(info) it’s a bit less clear that it’s just applying serialization parameters – a function call could well be doing just about anything after all. Leaving it as WithParams(...) makes it clear that it’s just applying serialization params.

Not sure that’s necessarily ideal, but that’s what I was thinking, fwiw.

maflcko commented at 10:40 am on September 14, 2023: member

Yeah, up to you. I personally don’t see a risk in s << ser_params(addr); and it is shorter and consistent with the s << V1(addr); stuff. So I’ll use it in new code, and potentially do the scripted-diff at some point, but no strong opinion.

TheCharlatan approved

TheCharlatan commented at 11:01 am on September 15, 2023: contributor

ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0

I personally don’t see a risk in s « ser_params(addr); and it is shorter and consistent with the s « V1(addr); stuff.

Also would have preferred s << ser_params(addr);.

fanquake merged this on Sep 15, 2023

fanquake closed this on Sep 15, 2023

ajtowns commented at 7:06 pm on September 15, 2023: contributor

Also would have preferred s << ser_params(addr);.

Maybe add this to #28451 when rebasing it?

Frank-GER referenced this in commit 775a2bcf9d on Sep 19, 2023

ajtowns commented at 5:50 am on October 27, 2023: contributor

Also would have preferred s << ser_params(addr);. @TheCharlatan You might want to review #28503

bitcoin locked this on Oct 26, 2024

refactor: Serialization parameter cleanups #28473

Reviews

Conflicts