[UniValue] replace global function find_value() with UniValue::findValue() #6580

pull jonasschnelli wants to merge 2 commits into bitcoin:master from jonasschnelli:2015/08/univalue_find_value changing 12 files +66 −58

jonasschnelli commented at 7:07 AM on August 21, 2015: contributor

The global function find_value() was implemented in UniValue to allow compatibility with JsonSpirit. This is no longer required and we should therefore move it to the UniValue object context.
jonasschnelli force-pushed on Aug 21, 2015
jonasschnelli force-pushed on Aug 21, 2015
laanwj added the label Refactoring on Aug 21, 2015
laanwj commented at 8:22 AM on August 21, 2015: member

Agree on the concept, but this diff is huge. I'd rather postpone it a bit to make sure other pulls don't suffer.
jonasschnelli commented at 8:26 AM on August 21, 2015: contributor

Merging this would produce trivial rebases because the change is very simple s/find_value(<obj>, <key>)/<obj>.findValue(<key>)/. Even if new code in PRs use find_value(), it's very trivial.

But also no rush in merging this, i can keep this rebased until there is a point in time where not much is going on.
jgarzik commented at 1:42 PM on August 21, 2015: contributor

ut ACK
dcousens commented at 2:43 AM on August 22, 2015: contributor

utACK, LGTM.
in src/univalue/univalue.cpp:None in a17ba06c95 outdated
```
 163 | @@ -164,6 +164,19 @@ int UniValue::findKey(const std::string& key) const
 164 |      return -1;
 165 |  }
 166 |  
 167 | +const UniValue UniValue::findValue(const std::string& key) const
 168 | +{
 169 | +    if (typ != VOBJ)
 170 | +        return NullUniValue;
```
dcousens commented at 2:45 AM on August 22, 2015:

This is the only real non-trivial change, it now defines the behaviour more specifically for non-VOBJ. Wouldn't this have worked already as keys would be empty?

jonasschnelli commented at 12:35 PM on August 23, 2015:

Agreed. This check (if (typ != VOBJ)) is not really necessary...

laanwj commented at 10:10 AM on August 24, 2015:

Is it a bug if this happens? I'd say findValue shouldn't be called for non-VOBJ in the first place. This is similar to calling get_bool on a non-VBOOL. So maybe it should also raise a std::runtime_error.

laanwj commented at 10:11 AM on August 24, 2015:

(but separating this from the trivial change makes sense)

dcousens commented at 10:48 AM on August 24, 2015:

I'd say throwing std::runtime_error would be fine, but agreed, separation from the trivial change should allow for merge.
jonasschnelli force-pushed on Aug 24, 2015
jonasschnelli commented at 11:26 AM on August 24, 2015: contributor

Removed the VOBJ check; only trivial changes now.
in src/univalue/univalue.h:None in 66e74f1240 outdated
```
 153 | @@ -154,7 +154,8 @@ class UniValue {
 154 |      bool push_back(std::pair<std::string,UniValue> pear) {
 155 |          return pushKV(pear.first, pear.second);
 156 |      }
 157 | -    friend const UniValue& find_value( const UniValue& obj, const std::string& name);
 158 | +    //!return a value if key was found (only supports VOBJ types)
```
dcousens commented at 11:34 AM on August 24, 2015:

is this comment out of date now?

jonasschnelli commented at 11:39 AM on August 24, 2015:

I would say this comment is appropriate. The keys vector is only >0 for VOBJ objects.

dcousens commented at 12:37 PM on August 24, 2015:

I feel like the appropriate comment would be only useful for VOBJ types, as technically the function is still "supported", and the behaviour is well defined. A nit.
dcousens commented at 11:34 AM on August 24, 2015: contributor

utACK
jonasschnelli force-pushed on Sep 14, 2015
jonasschnelli commented at 6:33 PM on September 14, 2015: contributor

Rebased.
dcousens commented at 9:54 PM on September 14, 2015: contributor

utACK

[UniValue] replace global function find_value with UniValue::findValue()

The global function `find_value()` was implemented in UniValue to allow compatibility with JsonSpirit. This is no longer required and we should therefore move it to the UniValue object context.

2ab2bc3117

jonasschnelli force-pushed on Sep 24, 2015
jonasschnelli commented at 3:07 PM on September 24, 2015: contributor

Rebased. This should be easy to review.

The function itself is move-only.

The rest of the code just follows search and replace of s/find_value(<obj>, <key>)/<obj>.findValue(<key>).

MarcoFalke commented at 1:35 PM on September 25, 2015: member

@jonasschnelli travis complains in case you didn't notice:

httprpc.cpp: In function ‘void JSONErrorReply(HTTPRequest*, const UniValue&, const UniValue&)’:

httprpc.cpp:62:43: error: ‘find_value’ was not declared in this scope

[UniValue] add UniValue::findValue() tests cc1e72f95d
jonasschnelli force-pushed on Sep 25, 2015
jonasschnelli commented at 1:50 PM on September 25, 2015: contributor

@MarcoFalke: Thanks! It was a rebase issue. Just fixed.
jgarzik commented at 10:06 AM on October 1, 2015: contributor
Strongly in favor of this, but due to size of diff, this should go in at a scheduled point when other "tree stirring" occurs:
- right after univalue subtree merge
- during a lot of libconsensus refactor merges etc.
jonasschnelli commented at 2:46 PM on October 1, 2015: contributor

This needs to go upstream now. Might come down again when we upgrade UniValue in this repo. Closing.
jonasschnelli closed this on Oct 1, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels