univalue: Remove unused and confusing set*() return value #25736
Conversation
There was a problem hiding this comment.
ACK fa7bef2
- 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.
|
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. |
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! |
Yeah, I think if a return code is not used where it should, it should be marked with |
Thanks for the explanation! Let me see if I can find some! |
There was a problem hiding this comment.
ACK fa7bef2.
I verified that return values were not being used.
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. |
The value is:
[[nodiscard]]true, unless a num-string is setInstead of adding
[[nodiscard]], throw when setting is not possible.