univalue: Remove confusing getBool method #26673

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/nobool changing 4 files +5 −6

ryanofsky commented at 3:13 PM on December 9, 2022: contributor

Drop UniValue::getBool method because it is easy to confuse with the UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool, getBool doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.

The getBool method is also redundant because it is an alias for isTrue. There were only 5 getBool() calls in the codebase, so this commit replaces them with isTrue() or get_bool() calls as appropriate.

These changes were originally made by MarcoFalke in #26213 but were dropped to limit the scope of that PR.

univalue: Remove confusing getBool method

Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.

The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.

These changes were originally made by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the
scope of that PR.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

293849a260

DrahtBot commented at 3:13 PM on December 9, 2022: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK sipa, w0xlt, justinpickering, hebasto, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label RPC/REST/ZMQ on Dec 9, 2022
hebasto commented at 3:15 PM on December 9, 2022: member

Concept ACK.

There were only 5 getBool() calls in the codebase...

And 4 of them are in tests.
justinpickering commented at 5:52 PM on December 9, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/26673/commits/293849a26088a16f6187dfdc36bef1d4d83ebb9d
sipa commented at 8:05 PM on December 9, 2022: member

utACK 293849a26088a16f6187dfdc36bef1d4d83ebb9d
w0xlt approved
w0xlt commented at 8:12 PM on December 9, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26673/commits/293849a26088a16f6187dfdc36bef1d4d83ebb9d
hebasto approved
hebasto commented at 10:21 PM on December 9, 2022: member
ACK 293849a26088a16f6187dfdc36bef1d4d83ebb9d, also verified that the removed getBool method is not mentioned in any docs:
```
$ git grep getBool | wc -l
0
```
furszy approved
furszy commented at 12:41 AM on December 10, 2022: member

ACK 293849a2

First glance, seems that a good follow-up would be to minimize the isFalse and isTrue methods usage scope as well. Probably only SettingToString and SettingToInt requires them.
ryanofsky commented at 1:54 AM on December 10, 2022: contributor

First glance, seems that a good follow-up would be to minimize the isFalse and isTrue methods usage scope as well. Probably only SettingToString and SettingToInt requires them.

~Would recommend looking at #26213 for this~. It removes some bad uses of isTrue (all the bad ones IMO)

EDIT: NVM I see you reviewed it already!
furszy commented at 2:45 AM on December 10, 2022: member

First glance, seems that a good follow-up would be to minimize the isFalse and isTrue methods usage scope as well. Probably only SettingToString and SettingToInt requires them.

~Would recommend looking at #26213 for this~. It removes some bad uses of isTrue (all the bad ones IMO)

EDIT: NVM I see you reviewed it already!

Actually, I forgot that one hehe. Thanks for the reminder!
fanquake merged this on Dec 10, 2022
fanquake closed this on Dec 10, 2022
sidhujag referenced this in commit 70af601862 on Dec 10, 2022
bitcoin locked this on Dec 10, 2023