Remove unused CSubNet serialize code#22375
Conversation
There was a problem hiding this comment.
ACK fa0dff5 code review, debug-built and tested after rebasing to master, ran unit tests and sub_net_deserialize fuzzer briefly
$ FUZZ=sub_net_deserialize src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/sub_net_deserialize
.../...
#103509 REDUCE cov: 352 ft: 544 corp: 51/1064b lim: 932 exec/s: 7393 rss: 205Mb L: 20/129 MS: 2 ShuffleBytes-EraseBytes-
#131072 pulse cov: 352 ft: 544 corp: 51/1064b lim: 1201 exec/s: 7710 rss: 224Mb
#150330 REDUCE cov: 352 ft: 544 corp: 51/1063b lim: 1391 exec/s: 7516 rss: 237Mb L: 6/129 MS: 1 EraseBytes-
#185891 REDUCE cov: 353 ft: 546 corp: 52/1085b lim: 1741 exec/s: 7745 rss: 261Mb L: 22/129 MS: 1 CMP- DE: "\xfd\x87\xd8~\xebC"-
#262144 pulse cov: 353 ft: 546 corp: 52/1085b lim: 2491 exec/s: 7710 rss: 313Mb
#524288 pulse cov: 353 ft: 546 corp: 52/1085b lim: 5105 exec/s: 8322 rss: 489Mb
#1048576 pulse cov: 353 ft: 546 corp: 52/1085b lim: 10320 exec/s: 8192 rss: 522Mb
|
Concept ACK Unused code should be removed. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| try { | ||
| { | ||
| int version; | ||
| ds >> version; | ||
| ds.SetVersion(version); | ||
| } | ||
| ds >> CSubNet{}; | ||
| } catch (const std::ios_base::failure&) { |
There was a problem hiding this comment.
to limit the scope of version and to put it in its own section. An alternative would be add a newline after ds.SetVersion and not care about the scope.
|
The |
|
Done in #22570 |
|
Concept ACK (as it doesn't get rid of a lot of code like this, I have a slight preference for removing the serialization and deserialization code at the same time, but no opposition to doing it like this either) |
|
Indeed. Please review (ACK/NACK) #22570 first. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
8 participants
The code is complicated and confusing. Now that it is unused, remove it to avoid the risk of using it later.