Page MenuHomePhabricator

Resolve FIXMEs in src/net.cpp (pass config through CConnman)
ClosedPublic

Authored by deadalnix on Jul 10 2017, 15:06.

Details

Summary

The config is added to CConnman so that its member functions can access it.

Test Plan

make check
../qa/pull-tester/rpc-tests.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJul 10 2017, 15:06
src/net.cpp
2267 ↗(On Diff #772)

This temporary variable should probably be removed, it's not really useful.

Remove cfg_ptr local variable

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.

deadalnix requested changes to this revision.Jul 10 2017, 18:12
This revision now requires changes to proceed.Jul 10 2017, 18:12
src/net.h
301 ↗(On Diff #773)

I first tried references for all, and ended up fighting the compiler on the thread functions.
It had something to do with boost::noncopyable if I remember correctly - I'd have to do another test to be sure of the error messages.

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.

freetrader edited edge metadata.

Add config to CConnman via constructor

Removes the need to pass the config down to other member functions.

freetrader retitled this revision from Resolve FIXMEs in src/net.cpp (pass config through) to Resolve FIXMEs in src/net.cpp (pass config through CConnman).Jul 11 2017, 17:18
freetrader edited the summary of this revision. (Show Details)
freetrader added inline comments.
src/net.h
303 ↗(On Diff #773)

Done in latest update.

src/util.h
263 ↗(On Diff #773)

No more need since a pointer to config has been added to CConnman.

Other than that, it would have been difficult since TraceThreadWithConfig was a template.

To remove the forward class declaration in util.h which is no longer needed.

Remove the forward class declaration which is no longer needed.

deadalnix requested changes to this revision.Jul 13 2017, 12:05

Back on your queue.

Please tell me how adding the config to CConMan goes.

This revision now requires changes to proceed.Jul 13 2017, 12:05

You can also pass by ref using std::thread using std::ref .

freetrader edited edge metadata.

Use references instead of pointer

deadalnix requested changes to this revision.Aug 18 2017, 21:54
deadalnix added inline comments.
This revision now requires changes to proceed.Aug 18 2017, 21:54
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.

freetrader edited edge metadata.

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.

deadalnix edited reviewers, added: freetrader; removed: deadalnix.
This revision is now accepted and ready to land.Aug 21 2017, 07:37
This revision was automatically updated to reflect the committed changes.