Drop unused GetType() from CSizeComputer #13558

pull Empact wants to merge 2 commits into bitcoin:master from Empact:c-size-computer changing 18 files +38 −46

Empact commented at 5:08 AM on June 28, 2018: member

Based on conversation in #13462, it seems the serialization GetType has very narrow use/effect. In every case except for CAddress, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type.

This removes the unused GetType method from CSizeComputer as a step to further narrowing that use.
fanquake added the label Refactoring on Jun 28, 2018
DrahtBot commented at 9:27 AM on June 28, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #14191 ([RPC] decodeblock by instagibbs)
- #13359 (Directly operate with CMutableTransaction by MarcoFalke)
- #13307 (Replace coin selection fallback strategy with Single Random Draw by achow101)
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.
sipa commented at 5:38 PM on June 28, 2018: member

utACK 60cf69ee7855b8d5c86d3ed41c47f764ccee5095. Nice simplification.
laanwj commented at 11:55 AM on July 10, 2018: member

I wonder if this is slightly confusing as long as long as it still uses the type parameter in the rest of (de)serialization.

Long term I do think that it makes sense to get rid of it altogether. Instead of passing a type into the serialization it'd be more clear to use a subclass or proxy object if different kinds of (de)serialization are wanted.
DrahtBot closed this on Aug 25, 2018
DrahtBot commented at 8:55 PM on August 25, 2018: member

The last travis run for this pull request was 58 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot reopened this on Aug 25, 2018
DrahtBot added the label Needs rebase on Aug 29, 2018
Empact force-pushed on Aug 30, 2018
Empact commented at 5:57 AM on August 30, 2018: member

Rebased for #13792
Empact force-pushed on Aug 30, 2018
DrahtBot removed the label Needs rebase on Aug 30, 2018
Drop unused GetType() from CSizeComputer da74db0940

Drop minor GetSerializeSize template

Now that `GetType()` is not propagated, the benefits are not worth the code.

893628be01

in src/serialize.h:906 in c77a66b551 outdated

 900 | @@ -901,10 +901,9 @@ class CSizeComputer
 901 |  protected:
 902 |      size_t nSize;
 903 |  
 904 | -    const int nType;
 905 |      const int nVersion;
 906 |  public:
 907 | -    CSizeComputer(int nTypeIn, int nVersionIn) : nSize(0), nType(nTypeIn), nVersion(nVersionIn) {}
 908 | +    CSizeComputer(int nVersionIn) : nSize(0), nVersion(nVersionIn) {}

practicalswift commented at 7:46 PM on September 5, 2018:

Should be explicit? :-)

laanwj commented at 4:54 PM on September 10, 2018:

yes, that's good practice for one-argument constructors (sorry for fat fingering the resolve button)

Empact force-pushed on Sep 11, 2018
Empact commented at 4:59 AM on September 11, 2018: member

Made the now single-arg constructor explicit
laanwj merged this on Sep 11, 2018
laanwj closed this on Sep 11, 2018
laanwj referenced this in commit bcffd8743e on Sep 11, 2018
Empact deleted the branch on Sep 23, 2018
kallewoof commented at 1:32 AM on October 6, 2018: member

Edit: Removed. Not really fair of me to ask such a thing. Still not sure how to fix the code but will work it out myself.
furszy referenced this in commit 5c93f159bc on Jul 5, 2021
random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#13462 Make SER_GETHASH implicit for CHashWriter and SerializeHash