refactor: Set RPCArg options with designated initializers #26074

MarcoFalke commented at 11:26 AM on September 13, 2022: member

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

fanquake added the label Refactoring on Sep 13, 2022

MarcoFalke force-pushed on Sep 13, 2022

in src/rpc/util.h:142 in fa53da86a1 outdated

 136 | @@ -137,6 +137,12 @@ enum class OuterType {
 137 |      NONE, // Only set on first recursion
 138 |  };
 139 |  
 140 | +struct RPCArgOptions {
 141 | +    std::string oneline_description{};   //!< Should be empty unless it is supposed to override the auto-generated summary line
 142 | +    std::vector<std::string> type_str{}; //!< Should be empty unless it is supposed to override the auto-generated type strings. Vector length is either 0 or 2, m_type_str.at(0) will override the type of the value in a key-value pair, m_type_str.at(1) will override the type in the argument description.

stickies-v commented at 3:59 PM on September 13, 2022:

    std::vector<std::string> type_str{}; //!< Should be empty unless it is supposed to override the auto-generated type strings. Vector length is either 0 or 2, type_str.at(0) will override the type of the value in a key-value pair, type_str.at(1) will override the type in the argument description.

There's also a now outdated reference to m_oneline_description at https://github.com/bitcoin/bitcoin/blob/fa53da86a1fd46d06bb2101d3b795ca8e5dc29cc/src/rpc/util.h#L228

MarcoFalke commented at 4:37 PM on September 13, 2022:

Thx, fixed!

in src/rpc/util.h:191 in fa53da86a1 outdated

 192 |          const Fallback fallback,
 193 |          const std::string description,
 194 | -        const std::string oneline_description = "",
 195 | -        const std::vector<std::string> type_str = {},
 196 | -        const bool hidden = false)
 197 | +        RPCArgOptions opts = {})

stickies-v commented at 4:02 PM on September 13, 2022:

Since these are always (I think?) rvalue refs, could avoid copying? + for the other ctor

        RPCArgOptions&& opts = {})

MarcoFalke commented at 4:34 PM on September 13, 2022:

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. a8f69541ad53a76d5f69044ba829069d225a4af1)

stickies-v commented at 6:03 PM on September 13, 2022:

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.

MarcoFalke commented at 6:19 PM on September 13, 2022:

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 :-1: and :+1:

stickies-v approved

stickies-v commented at 4:06 PM on September 13, 2022: contributor

ACK fa53da86a

Couldn't find any behaviour change. Makes the code more readable at reasonable increased verbosity.

rpc: Set RPCArg options with designated initializers fa2c72dda0

MarcoFalke force-pushed on Sep 13, 2022

stickies-v approved

stickies-v commented at 6:05 PM on September 13, 2022: contributor

re-ACK fa2c72dda09f9b51332f6c7953ae81e573cc834f

DrahtBot commented at 9:32 AM on September 14, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25093 (rpc: Check for omitted, but required parameters by MarcoFalke)
#23549 (Add scanblocks RPC call (attempt 2) by jamesob)

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.

MarcoFalke merged this on Sep 30, 2022

MarcoFalke closed this on Sep 30, 2022

MarcoFalke deleted the branch on Sep 30, 2022

sidhujag referenced this in commit eec3be94c5 on Oct 4, 2022

bitcoin locked this on Sep 30, 2023