The config is added to CConnman so that its member functions can access it.
Details
- Reviewers
sickpig awemany dagurval CCulianu freetrader - Group Reviewers
Restricted Project - Commits
- rSTAGINGf17b1957b9d2: Resolve FIXMEs in src/net.cpp (pass config through CConnman)
rABCf17b1957b9d2: Resolve FIXMEs in src/net.cpp (pass config through CConnman)
make check
../qa/pull-tester/rpc-tests.py
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- net_fixmes
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 493 Build 493: arc lint + arc unit
Event Timeline
src/net.cpp | ||
---|---|---|
2267 ↗ | (On Diff #772) | This temporary variable should probably be removed, it's not really useful. |
src/net.h | ||
---|---|---|
301 ↗ | (On Diff #773) | In some of these you pass by ref, and other by pointer. Why ? |
303 ↗ | (On Diff #773) | Maybe the config can be made a member of the CConMan ? |
src/util.h | ||
18 ↗ | (On Diff #773) | Remove this include, you don't need it to declare. You can u=include in util.cpp once the implementation moved there. |
263 ↗ | (On Diff #773) | Implement in util.cpp so changing it doesn't require to recompile half of the universe. |
src/net.h | ||
---|---|---|
301 ↗ | (On Diff #773) | I first tried references for all, and ended up fighting the compiler on the thread functions. For the methods called from the threads, I could follow the existing style of using references. |
303 ↗ | (On Diff #773) | I'll have a look at this option. |
src/util.h | ||
263 ↗ | (On Diff #773) | Ok, will give that a try. |
Add config to CConnman via constructor
Removes the need to pass the config down to other member functions.
src/net.h | ||
---|---|---|
342 ↗ | (On Diff #1185) |
src/net.h | ||
---|---|---|
342 ↗ | (On Diff #1185) | Well, whenever I see this in a class I think "ok, that's a pointer that can never dangle or a reference to some global object that never goes away -- aka a singleton". I actually think it's ok if you use it with those semantics. And if the semantics change it's easy enough to change to a pointer. I think it's easier to read code that has a reference to something that is global than a pointer. A pointer implies dynamism -- there could me more than 1, and the location can change. This is a reference to a singleton. It's a perfectly ok idiom to do this, FWIW. It implies a lack of dynamism. It can't dangle. It's referring to a singleton in this case, so it's always defined. I like it. A lot of code I have see does member references exactly in this situation -- specifically to illustrate that it's a reference to a singleton. There is information in that choice of notation, basically. That's the convention I am use to. The stackoverflow guys have their opinions of course about this, but I disagree with them. For singletons actually references are good because they make you realize "this thing might be a singleton". And you stop, and check, and find out it really is. So the notation itself expresses something. I hate to contradict you on this -- but I'm ok with freetrader's choice here. The fact that it's a reference tells the reader of the code that it might be a singleton or at least a very long-lived non-dynamic object -- and in fact it is. |
Back to review for more feedback on what changes exactly are required for this diff.
As stated on Slack, I can think of 3 ways of changing this, and the only one not already tried in this Diff would be to duplicate the Config object entirely within CConnMan which seems to defeat the point of the singleton, as @CCulianu mentions.
So I don't have enough information to go on to know which change to make to this Diff.
From discussions on Slack, it's become clear that a reference member is to be avoided and a pointer used instead.
src/net.h | ||
---|---|---|
342 ↗ | (On Diff #1185) | If you argue class A should be designed by baking in assumptions about class B's implementation into it, you already lost the argument. This defeat the whole point of having abstraction in the first place. |
src/net.h | ||
---|---|---|
342 ↗ | (On Diff #1185) | No, I have not lost the argument. You don't understand the argument, or at least pretend not to, or perhaps just are too lazy to read it and write back an actual reply, so you play dumb and write back short ignorant replies and declare victory for yourself without actually offering any real discussion or intelligence. Go back and really read what I wrote. |