univalue: Remove unused and confusing set*() return value #25736

The value is:

• currently unused, and useless without [[nodiscard]]
• confusing, because it is always true, unless a num-string is set

Instead of adding [[nodiscard]], throw when setting is not possible.

ACK fa7bef2e80c9c290b3e97114cfa7afdea5cbd53c

• I agree with removing redundant return values, as it makes the code cleaner and clearer to understand.
• Verified that all the appropriate changes are done for each set*() function so the code could run properly.
• I successfully compiled and ran the bitcoind on this PR.

Sidenote:

• I have observed a similar thing for some time. Some functions are declared with a return value, like a bool. But when they are used, no variable is used to catch the return value.

For example:

bool setArray();

Which was converted to void setArray(); in this PR.

It was used in src/wallet/wallet.cpp and src/wallet/rpc/backup.cpp files without providing any variables to catch the returned bool value.

util::SettingsValue setting_value = chain.getRwSetting("wallet");
if (!setting_value.isArray()) setting_value.setArray();
for (const util::SettingsValue& value : setting_value.getValues()) {
    if (value.isStr() && value.get_str() == wallet_name) return true;
}

and,

std::vector<UniValue> results = response.getValues();
response.clear();
response.setArray();
size_t i = 0;

So was this a conscious decision by the programmer, and is this an acceptable behavior? If not, should we work towards removing this discrepancy?

I would greatly appreciate fellow contributors' responses to my query.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25714 (univalue: Avoid std::string copies by MarcoFalke)

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.

If not, should we work towards removing this discrepancy?

Isn't that what this pull is doing?

Isn't that what this pull is doing?

Yes. But I also observed similar redundancies elsewhere as well. Not sure if I can recall exactly where.

I was asking to ensure this behavior is indeed a discrepancy before hunting for them in the codebase.

Thanks for the confirmation, btw!

Yes. But I also observed similar redundancies elsewhere as well. Not sure if I can recall exactly where. I was asking to ensure this behavior is indeed a discrepancy before hunting for them in the codebase.

Yeah, I think if a return code is not used where it should, it should be marked with [[nodiscard]]. If it shouldn't be used, it should be replaced with void.

Yeah, I think if a return code is not used where it should, it should be marked with [[nodiscard]]. If it isn't used, it should be replaced with the void.

Thanks for the explanation! Let me see if I can find some!

ACK fa7bef2e80c9c290b3e97114cfa7afdea5cbd53c. I verified that return values were not being used.

Also, should we enforce the clang-tidy check bugprone-unused-return-value?

Also, should we enforce the clang-tidy check bugprone-unused-return-value?

Sounds good, but the check only runs on std-lib functions by default, so it seems unrelated to the changes here.