refactor: Set RPCArg options with designated initializers #26074
Conversation
fa36a3a to
fa53da8
Compare
There was a problem hiding this comment.
ACK fa53da8
Couldn't find any behaviour change. Makes the code more readable at reasonable increased verbosity.
| const std::string oneline_description = "", | ||
| const std::vector<std::string> type_str = {}, | ||
| const bool hidden = false) | ||
| RPCArgOptions opts = {}) |
There was a problem hiding this comment.
Since these are always (I think?) rvalue refs, could avoid copying? + for the other ctor
| RPCArgOptions opts = {}) | |
| RPCArgOptions&& opts = {}) |
There was a problem hiding this comment.
I don't think adding && here will avoid a copy. && is just a reference type that forces the caller to pass in an rvalue(ref). This would only avoid a copy if it was an lvalue, cast to an rvalue. Though, in that case we might want to re-use the lvalue and not force the caller to move it (or explicitly copy it), so I think RPCArgOptions is fine here.
While both are fine here, a pure r-value and an r-value ref, I think it is fine to only use && in cases where a copy can be avoided and the copy would be costly (e.g. a8f6954)
There was a problem hiding this comment.
I think you're right that there is no need for && here, but I think the reason is that since c++17 mandatory copy elision avoids copying the temporary in the first place?
Without copy elision, I think passing by value instead of rvalue ref would have required a constructing and then copy-constructing operation, as pass by value always means copying (except with elision)?
Drifting off-topic, so you can consider this resolved. Don't want to waste anyone's time.
There was a problem hiding this comment.
I guess you are right that for arguments copy elision is not mandatory, but I'd be surprised to see a compiler that wouldn't do it voluntarily.
Though, happy to change to &&. If other reviewers see this they can use 👎 and 👍
fa53da8 to
fa2c72d
Compare
|
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. |
…tializers fa2c72d rpc: Set RPCArg options with designated initializers (MacroFake) Pull request description: For optional constructor arguments, use a new struct. This comes with two benefits: * Earlier unused optional arguments can be omitted * Designated initializers can be used ACKs for top commit: stickies-v: re-ACK fa2c72d Tree-SHA512: 2a0619548187cc7437fee2466ac4780746490622f202659f53641be01bc2a1fea4416d1a77f3e963bf7c4cce62899b61fab0b9683440cf82f68be44f63826658
For optional constructor arguments, use a new struct. This comes with two benefits: