This PR is part of the process separation project.
CSubNet serialization code that was removed in #22570 fa4e6afdae7b82df638b60edf37ac36d57a8cb4f was needed by multiprocess code to share ban map between gui and node processes.
Rather than adding it back, use suggestion from MarcoFalke #10102 (review) to use JSON serialization. This requires making BanMapToJson / BanMapFromJson functions public.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
git show --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space. Needs rebase.
CSubNet serialization code that was removed in
fa4e6afdae7b82df638b60edf37ac36d57a8cb4f was needed by multiprocess code
to share ban map between gui and node processes.
Rather than adding it back, use suggestion from MarcoFalke
<falke.marco@gmail.com>
https://github.com/bitcoin/bitcoin/pull/10102#discussion_r690922929 to
use JSON serialization. This requires making BanMapToJson /
BanMapFromJson functions public.
pr/ipc-banmap.1 -> pr/ipc-banmap.2, compare) due to conflict with #22849
5@@ -6,10 +6,55 @@
6 #define BITCOIN_NET_TYPES_H
7
8 #include <map>
9+#include <serialize.h>
pr/ipc-banmap.2 -> pr/ipc-banmap.3, compare) tweaking include
4@@ -5,11 +5,56 @@
5 #ifndef BITCOIN_NET_TYPES_H
6 #define BITCOIN_NET_TYPES_H
7
8+#include <cstdint>
9 #include <map>
10
11-class CBanEntry;
12 class CSubNet;
13+class UniValue;
Any reason not to use #include <univalue.h> here instead of class UniValue; (there is no circular dependency in this case)?
I think by default it should be preferred to include the header instead of doing the forward declaration. And if the forward declaration is really necessary (e.g. circular dependency, compilation speed concerns) then that should be in its own header that is included by “users”. For example, it is annoying to have to change struct Foo to class Foo with forward declarations spilled all over the place.
Any reason not to use
#include <univalue.h>here instead ofclass UniValue;(there is no circular dependency in this case)?I think by default it should be preferred to include the header instead of doing the forward declaration. And if the forward declaration is really necessary (e.g. circular dependency, compilation speed concerns) then that should be in its own header that is included by “users”. For example, it is annoying to have to change
struct Footoclass Foowith forward declarations spilled all over the place.
I have the opposite preference and prefer simple forward declarations whenever possible instead of includes. (Simple forward declarations meaning declarations like class Foo; or struct Foo; enum class Foo; not declarations for templates, typedefs, or std:: types). I don’t think forward declarations are just a workaround for circular dependencies. I think they also are important for keeping build times under control and for eliminating dependencies between modules while allowing them to pass data in an opaque but type safe way (avoiding void*). It’s possible my preference is out of date (it comes from working on projects using IWYU https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md#why-forward-declare), though, and it probably wouldn’t be a bad idea to have a linter or developer note for more consistency here if you wanted to follow up!
Alright, https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md#why-forward-declare mentions some pros and cons of forward declarations and also a workaround of the cons - a ‘forwarding headers’.
What about this: https://google.github.io/styleguide/cppguide.html#Forward_Declarations
- Forward declarations can hide a dependency, allowing user code to skip necessary recompilation when headers change.
- … Replacing an #include with a forward declaration can silently change the meaning of code …
I think each one of those two is very nasty on its own.
Did anybody bother to measure the compilation time improvement due to forward declarations? I suspect it may be insignificant.
- Forward declarations can hide a dependency, allowing user code to skip necessary recompilation when headers change.
- … Replacing an #include with a forward declaration can silently change the meaning of code …
I think each one of those two is very nasty on its own.
The first one would just result in link errors instead of compile errors. Not especially nasty as far as I can see. The second one seems like FUD. Is there a realistic example of this happening? Could the example happen accidentally, or would you have to go out of your way to engineer an it like in void* inheritance case? If this could happen accidentally, would basic sentient human code review or use of “grep” catch it?
There are practical benefits to using simple forward declarations like the non-template non-typedef non-std:: ones I write myself, or the ones created by the IWYU tool. Obviously there are specific situations where forward declarations are bad to use and the google style guide covers them well. But there is no reason to throw out the baby with the bathwater with another one of these “X considered harmful” proclamations. Especially since unlike other X’s, you can’t actually get rid forward includes if you want to write type-safe modular code.
I will also say that I like the use of forward declarations aesthetically and semantically. An include says “the code in this file depends on this code in this other file”. A forward declaration when it is properly used (something enforceable by IWYU) says “the code in this file does NOT depend on this type, and is just letting code in two other files that do you use the type communicate safely.”