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

MacroFake · MacroFake · commit 816ca01650f4 · 2022-08-02T12:30:49.000+02:00
fa7bef2 univalue: Remove unused and confusing set*() return value (MacroFake) Pull request description: 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. ACKs for top commit: shaavan: ACK fa7bef2 aureleoules: ACK fa7bef2. Tree-SHA512: 0d74f96f34cb93b66019ab75e12334c964630cc83434f22e58cc7a4fff2ee96a5767e42ab37f08acb67aeacba6811b09c75f1edc68d5e903ccfc59b1c82de891
diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
@@ -181,10 +181,10 @@ BOOST_AUTO_TEST_CASE(rpc_format_monetary_values)
     BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min()).write(), "-92233720368.54775808");
 }
 
-static UniValue ValueFromString(const std::string &str)
+static UniValue ValueFromString(const std::string& str) noexcept
 {
     UniValue value;
-    BOOST_CHECK(value.setNumStr(str));
+    value.setNumStr(str);
     return value;
 }
 
diff --git a/src/univalue/include/univalue.h b/src/univalue/include/univalue.h
@@ -47,16 +47,16 @@ class UniValue {
 
     void clear();
 
-    bool setNull();
-    bool setBool(bool val);
-    bool setNumStr(const std::string& val);
-    bool setInt(uint64_t val);
-    bool setInt(int64_t val);
-    bool setInt(int val_) { return setInt((int64_t)val_); }
-    bool setFloat(double val);
-    bool setStr(const std::string& val);
-    bool setArray();
-    bool setObject();
+    void setNull();
+    void setBool(bool val);
+    void setNumStr(const std::string& val);
+    void setInt(uint64_t val);
+    void setInt(int64_t val);
+    void setInt(int val_) { return setInt(int64_t{val_}); }
+    void setFloat(double val);
+    void setStr(const std::string& val);
+    void setArray();
+    void setObject();
 
     enum VType getType() const { return typ; }
     const std::string& getValStr() const { return val; }
diff --git a/src/univalue/lib/univalue.cpp b/src/univalue/lib/univalue.cpp
@@ -23,19 +23,17 @@ void UniValue::clear()
     values.clear();
 }
 
-bool UniValue::setNull()
+void UniValue::setNull()
 {
     clear();
-    return true;
 }
 
-bool UniValue::setBool(bool val_)
+void UniValue::setBool(bool val_)
 {
     clear();
     typ = VBOOL;
     if (val_)
         val = "1";
-    return true;
 }
 
 static bool validNumStr(const std::string& s)
@@ -46,18 +44,18 @@ static bool validNumStr(const std::string& s)
     return (tt == JTOK_NUMBER);
 }
 
-bool UniValue::setNumStr(const std::string& val_)
+void UniValue::setNumStr(const std::string& val_)
 {
-    if (!validNumStr(val_))
-        return false;
+    if (!validNumStr(val_)) {
+        throw std::runtime_error{"The string '" + val_ + "' is not a valid JSON number"};
+    }
 
     clear();
     typ = VNUM;
     val = val_;
-    return true;
 }
 
-bool UniValue::setInt(uint64_t val_)
+void UniValue::setInt(uint64_t val_)
 {
     std::ostringstream oss;
 
@@ -66,7 +64,7 @@ bool UniValue::setInt(uint64_t val_)
     return setNumStr(oss.str());
 }
 
-bool UniValue::setInt(int64_t val_)
+void UniValue::setInt(int64_t val_)
 {
     std::ostringstream oss;
 
@@ -75,37 +73,32 @@ bool UniValue::setInt(int64_t val_)
     return setNumStr(oss.str());
 }
 
-bool UniValue::setFloat(double val_)
+void UniValue::setFloat(double val_)
 {
     std::ostringstream oss;
 
     oss << std::setprecision(16) << val_;
 
-    bool ret = setNumStr(oss.str());
-    typ = VNUM;
-    return ret;
+    return setNumStr(oss.str());
 }
 
-bool UniValue::setStr(const std::string& val_)
+void UniValue::setStr(const std::string& val_)
 {
     clear();
     typ = VSTR;
     val = val_;
-    return true;
 }
 
-bool UniValue::setArray()
+void UniValue::setArray()
 {
     clear();
     typ = VARR;
-    return true;
 }
 
-bool UniValue::setObject()
+void UniValue::setObject()
 {
     clear();
     typ = VOBJ;
-    return true;
 }
 
 void UniValue::push_back(const UniValue& val_)
diff --git a/src/univalue/test/object.cpp b/src/univalue/test/object.cpp
@@ -45,7 +45,7 @@ void univalue_constructor()
     BOOST_CHECK_EQUAL(v3.getValStr(), "foo");
 
     UniValue numTest;
-    BOOST_CHECK(numTest.setNumStr("82"));
+    numTest.setNumStr("82");
     BOOST_CHECK(numTest.isNum());
     BOOST_CHECK_EQUAL(numTest.getValStr(), "82");
 
@@ -93,33 +93,33 @@ void univalue_push_throw()
 void univalue_typecheck()
 {
     UniValue v1;
-    BOOST_CHECK(v1.setNumStr("1"));
+    v1.setNumStr("1");
     BOOST_CHECK(v1.isNum());
     BOOST_CHECK_THROW(v1.get_bool(), std::runtime_error);
 
     {
         UniValue v_negative;
-        BOOST_CHECK(v_negative.setNumStr("-1"));
+        v_negative.setNumStr("-1");
         BOOST_CHECK_THROW(v_negative.getInt<uint8_t>(), std::runtime_error);
         BOOST_CHECK_EQUAL(v_negative.getInt<int8_t>(), -1);
     }
 
     UniValue v2;
-    BOOST_CHECK(v2.setBool(true));
+    v2.setBool(true);
     BOOST_CHECK_EQUAL(v2.get_bool(), true);
     BOOST_CHECK_THROW(v2.getInt<int>(), std::runtime_error);
 
     UniValue v3;
-    BOOST_CHECK(v3.setNumStr("32482348723847471234"));
+    v3.setNumStr("32482348723847471234");
     BOOST_CHECK_THROW(v3.getInt<int64_t>(), std::runtime_error);
-    BOOST_CHECK(v3.setNumStr("1000"));
+    v3.setNumStr("1000");
     BOOST_CHECK_EQUAL(v3.getInt<int64_t>(), 1000);
 
     UniValue v4;
-    BOOST_CHECK(v4.setNumStr("2147483648"));
+    v4.setNumStr("2147483648");
     BOOST_CHECK_EQUAL(v4.getInt<int64_t>(), 2147483648);
     BOOST_CHECK_THROW(v4.getInt<int>(), std::runtime_error);
-    BOOST_CHECK(v4.setNumStr("1000"));
+    v4.setNumStr("1000");
     BOOST_CHECK_EQUAL(v4.getInt<int>(), 1000);
     BOOST_CHECK_THROW(v4.get_str(), std::runtime_error);
     BOOST_CHECK_EQUAL(v4.get_real(), 1000);
@@ -146,55 +146,55 @@ void univalue_set()
     BOOST_CHECK(v.isNull());
     BOOST_CHECK_EQUAL(v.getValStr(), "");
 
-    BOOST_CHECK(v.setObject());
+    v.setObject();
     BOOST_CHECK(v.isObject());
     BOOST_CHECK_EQUAL(v.size(), 0);
     BOOST_CHECK_EQUAL(v.getType(), UniValue::VOBJ);
     BOOST_CHECK(v.empty());
 
-    BOOST_CHECK(v.setArray());
+    v.setArray();
     BOOST_CHECK(v.isArray());
     BOOST_CHECK_EQUAL(v.size(), 0);
 
-    BOOST_CHECK(v.setStr("zum"));
+    v.setStr("zum");
     BOOST_CHECK(v.isStr());
     BOOST_CHECK_EQUAL(v.getValStr(), "zum");
 
-    BOOST_CHECK(v.setFloat(-1.01));
+    v.setFloat(-1.01);
     BOOST_CHECK(v.isNum());
     BOOST_CHECK_EQUAL(v.getValStr(), "-1.01");
 
-    BOOST_CHECK(v.setInt((int)1023));
+    v.setInt(int{1023});
     BOOST_CHECK(v.isNum());
     BOOST_CHECK_EQUAL(v.getValStr(), "1023");
 
-    BOOST_CHECK(v.setInt((int64_t)-1023LL));
+    v.setInt(int64_t{-1023LL});
     BOOST_CHECK(v.isNum());
     BOOST_CHECK_EQUAL(v.getValStr(), "-1023");
 
-    BOOST_CHECK(v.setInt((uint64_t)1023ULL));
+    v.setInt(uint64_t{1023ULL});
     BOOST_CHECK(v.isNum());
     BOOST_CHECK_EQUAL(v.getValStr(), "1023");
 
-    BOOST_CHECK(v.setNumStr("-688"));
+    v.setNumStr("-688");
     BOOST_CHECK(v.isNum());
     BOOST_CHECK_EQUAL(v.getValStr(), "-688");
 
-    BOOST_CHECK(v.setBool(false));
+    v.setBool(false);
     BOOST_CHECK_EQUAL(v.isBool(), true);
     BOOST_CHECK_EQUAL(v.isTrue(), false);
     BOOST_CHECK_EQUAL(v.isFalse(), true);
     BOOST_CHECK_EQUAL(v.getBool(), false);
 
-    BOOST_CHECK(v.setBool(true));
+    v.setBool(true);
     BOOST_CHECK_EQUAL(v.isBool(), true);
     BOOST_CHECK_EQUAL(v.isTrue(), true);
     BOOST_CHECK_EQUAL(v.isFalse(), false);
     BOOST_CHECK_EQUAL(v.getBool(), true);
 
-    BOOST_CHECK(!v.setNumStr("zombocom"));
+    BOOST_CHECK_THROW(v.setNumStr("zombocom"), std::runtime_error);
 
-    BOOST_CHECK(v.setNull());
+    v.setNull();
     BOOST_CHECK(v.isNull());
 }
 
@@ -352,7 +352,7 @@ void univalue_object()
     BOOST_CHECK_EQUAL(obj.size(), 0);
     BOOST_CHECK_EQUAL(obj.getType(), UniValue::VNULL);
 
-    BOOST_CHECK_EQUAL(obj.setObject(), true);
+    obj.setObject();
     UniValue uv;
     uv.setInt(42);
     obj.__pushKV("age", uv);
@@ -419,7 +419,7 @@ void univalue_readwrite()
     BOOST_CHECK(!v.read("{} 42"));
 }
 
-int main (int argc, char *argv[])
+int main(int argc, char* argv[])
 {
     univalue_constructor();
     univalue_push_throw();
@@ -430,4 +430,3 @@ int main (int argc, char *argv[])
     univalue_readwrite();
     return 0;
 }
-

Original file line number	Diff line number	Diff line change
`@@ -181,10 +181,10 @@ BOOST_AUTO_TEST_CASE(rpc_format_monetary_values)`
`181`	`181`	`BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min()).write(), "-92233720368.54775808");`
`182`	`182`	`}`
`183`	`183`
`184`		`-static UniValue ValueFromString(const std::string &str)`
	`184`	`+static UniValue ValueFromString(const std::string& str) noexcept`
`185`	`185`	`{`
`186`	`186`	`UniValue value;`
`187`		`- BOOST_CHECK(value.setNumStr(str));`
	`187`	`+ value.setNumStr(str);`
`188`	`188`	`return value;`
`189`	`189`	`}`
`190`	`190`
Original file line number	Diff line number	Diff line change
`@@ -23,19 +23,17 @@ void UniValue::clear()`
`23`	`23`	`values.clear();`
`24`	`24`	`}`
`25`	`25`
`26`		`-bool UniValue::setNull()`
	`26`	`+void UniValue::setNull()`
`27`	`27`	`{`
`28`	`28`	`clear();`
`29`		`- return true;`
`30`	`29`	`}`
`31`	`30`
`32`		`-bool UniValue::setBool(bool val_)`
	`31`	`+void UniValue::setBool(bool val_)`
`33`	`32`	`{`
`34`	`33`	`clear();`
`35`	`34`	`typ = VBOOL;`
`36`	`35`	`if (val_)`
`37`	`36`	`val = "1";`
`38`		`- return true;`
`39`	`37`	`}`
`40`	`38`
`41`	`39`	`static bool validNumStr(const std::string& s)`
`@@ -46,18 +44,18 @@ static bool validNumStr(const std::string& s)`
`46`	`44`	`return (tt == JTOK_NUMBER);`
`47`	`45`	`}`
`48`	`46`
`49`		`-bool UniValue::setNumStr(const std::string& val_)`
	`47`	`+void UniValue::setNumStr(const std::string& val_)`
`50`	`48`	`{`
`51`		`- if (!validNumStr(val_))`
`52`		`- return false;`
	`49`	`+ if (!validNumStr(val_)) {`
	`50`	`+ throw std::runtime_error{"The string '" + val_ + "' is not a valid JSON number"};`
	`51`	`+ }`
`53`	`52`
`54`	`53`	`clear();`
`55`	`54`	`typ = VNUM;`
`56`	`55`	`val = val_;`
`57`		`- return true;`
`58`	`56`	`}`
`59`	`57`
`60`		`-bool UniValue::setInt(uint64_t val_)`
	`58`	`+void UniValue::setInt(uint64_t val_)`
`61`	`59`	`{`
`62`	`60`	`std::ostringstream oss;`
`63`	`61`
`@@ -66,7 +64,7 @@ bool UniValue::setInt(uint64_t val_)`
`66`	`64`	`return setNumStr(oss.str());`
`67`	`65`	`}`
`68`	`66`
`69`		`-bool UniValue::setInt(int64_t val_)`
	`67`	`+void UniValue::setInt(int64_t val_)`
`70`	`68`	`{`
`71`	`69`	`std::ostringstream oss;`
`72`	`70`
`@@ -75,37 +73,32 @@ bool UniValue::setInt(int64_t val_)`
`75`	`73`	`return setNumStr(oss.str());`
`76`	`74`	`}`
`77`	`75`
`78`		`-bool UniValue::setFloat(double val_)`
	`76`	`+void UniValue::setFloat(double val_)`
`79`	`77`	`{`
`80`	`78`	`std::ostringstream oss;`
`81`	`79`
`82`	`80`	`oss << std::setprecision(16) << val_;`
`83`	`81`
`84`		`- bool ret = setNumStr(oss.str());`
`85`		`- typ = VNUM;`
`86`		`- return ret;`
	`82`	`+ return setNumStr(oss.str());`
`87`	`83`	`}`
`88`	`84`
`89`		`-bool UniValue::setStr(const std::string& val_)`
	`85`	`+void UniValue::setStr(const std::string& val_)`
`90`	`86`	`{`
`91`	`87`	`clear();`
`92`	`88`	`typ = VSTR;`
`93`	`89`	`val = val_;`
`94`		`- return true;`
`95`	`90`	`}`
`96`	`91`
`97`		`-bool UniValue::setArray()`
	`92`	`+void UniValue::setArray()`
`98`	`93`	`{`
`99`	`94`	`clear();`
`100`	`95`	`typ = VARR;`
`101`		`- return true;`
`102`	`96`	`}`
`103`	`97`
`104`		`-bool UniValue::setObject()`
	`98`	`+void UniValue::setObject()`
`105`	`99`	`{`
`106`	`100`	`clear();`
`107`	`101`	`typ = VOBJ;`
`108`		`- return true;`
`109`	`102`	`}`
`110`	`103`
`111`	`104`	`void UniValue::push_back(const UniValue& val_)`