-
Notifications
You must be signed in to change notification settings - Fork 790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce support for and use automatic port binding in rpc_test
unit tests
#3576
Introduce support for and use automatic port binding in rpc_test
unit tests
#3576
Conversation
42686a6
to
618076c
Compare
9f0a878
to
6478246
Compare
Rebased this branch as well on top of its target and fixed build issues -- CC @dsiganos @thsfs @clemahieu, planning on getting it in soon to avoid conflicts and be able to push some more pending work that is depending on this one. Thanks. |
@@ -12,8 +12,8 @@ | |||
#include <nano/rpc/rpc_secure.hpp> | |||
#endif | |||
|
|||
nano::rpc::rpc (boost::asio::io_context & io_ctx_a, nano::rpc_config const & config_a, nano::rpc_handler_interface & rpc_handler_interface_a) : | |||
config (config_a), | |||
nano::rpc::rpc (boost::asio::io_context & io_ctx_a, nano::rpc_config config_a, nano::rpc_handler_interface & rpc_handler_interface_a) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to change this to pass by value instead of const ref? I changed it back and everything still compiles and runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be part of a different PR (later edit: opened it, it's here -- #3606) in which I tried to do only such things, but I must've got it wrong somewhere and it ended up here in this PR instead.
Change itself is fixing a clang-tidy warning with which I believe we agree -- e.g. always pass by value and move to and from when passing to something that keeps it as a member, as opposed to pass by reference when the receiver just keeps a reference? I'm about to open a PR (#3606) with a few more such changes wherever clang-tidy highlighted them, so please let me know if this is something that we generally avoid, although IMHO it helps with memory consumption and overall better code quality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case we want the ability to update config options so we want the reference behavior.
Do you mean reduced memory consumption by-reference or by-value?
I don't really agree with the pass-by-value first policy, I usually do the opposite which helps keep function calls referentially transparent since the function call doesn't imply copy operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As config
is an object and not a reference, updating config options via the reference would not work anyways.
I am talking about the following use cases:
- object
X
has aY&
member variable, then it should receive it in its constructor as a reference and who constructs it passes a reference - object
X
has aY
member variable, then receiving it in its constructor as a reference implies a copy from the reference to the member variable; instead, it's better to receive it by value, move from the value to the member, and who constructs it should alsonew X(std::move(y))
, ornew X(Y())
, which equals in two moves instead of a copy; worst case scenario is if the caller of the constructor does not move, then it's a copy at construction and a move at saving into member, but this is a programming error for the caller, usually it should only be 2 moves and no copy.
nano::rpc::rpc
falls into the 2
category, and all the other spots I was mentioning in my previous comment (#3606) are also from the 2
category. Do you agree with 2
? For 1
, of course, we use references, it would be neither efficient nor even correct (losing the original reference) to use values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that the member is an object rather than a reference.
I do see some logic in #2, I'll think on it a bit.
13d7f96
to
4519104
Compare
Rebased this one more time following the merge of #3554 into |
Basing this off of #3555 (which is itself based off of #3554) in order to make review easier and conflicts non-existent.
For #3554 I still have to make a pending code review addressing, but other than that everything is reviewable and mergeable after approval.