test: Add BOOST_REQUIRE to getters returning optional #14771

MarcoFalke commented at 11:41 PM on November 20, 2018: member

Usually the returned value is already checked for equality, but for sanity we might as well require that the getter successfully returned.

MarcoFalke added the label Tests on Nov 20, 2018

Empact commented at 12:33 AM on November 21, 2018: member

Do you mean to tag ReadConfigStream or GetValue NODISCARD as well? They could be.

GetKey has some unchecked uses in the codebase.

test: Add BOOST_REQUIRE to getters returning optional fa21ca09a8

MarcoFalke force-pushed on Nov 21, 2018

laanwj commented at 9:13 AM on November 21, 2018: member

utACK, I like the idea of adding NODISCARD annotations to make sure the return codes are not ignored.

practicalswift commented at 10:25 PM on November 21, 2018: contributor

utACK – NODISCARD is great!

in src/test/coins_tests.cpp:40 in fa21ca09a8

  36 | @@ -36,7 +37,7 @@ class CCoinsViewTest : public CCoinsView
  37 |      std::map<COutPoint, Coin> map_;
  38 |  
  39 |  public:
  40 | -    bool GetCoin(const COutPoint& outpoint, Coin& coin) const override
  41 | +    NODISCARD bool GetCoin(const COutPoint& outpoint, Coin& coin) const override

promag commented at 11:09 AM on November 22, 2018:

Sorry, but this seems like a random change, why is this necessary?

practicalswift commented at 12:50 PM on November 22, 2018:

I'll let @MarcoFalke answer on this specific case, but generally I think it makes perfect sense to add NODISCARD to functions that return a bool indicating success/failure. I think we should do that by default for new functions at least.

Making discarding opt-in will guard against potentially dangerous unintentional discards :-)

promag commented at 12:55 PM on November 22, 2018:

There are tons of that?

practicalswift commented at 1:04 PM on November 22, 2018:

@promag Let me re-phrase: I think we should add NODISCARD to functions – especially new functions – where discarding the return value might be a mistake.

Such as bool ParseFoo(const std::string& s, Foo *out);, etc.

Sounds reasonable? :-)

promag commented at 1:08 PM on November 22, 2018:

@practicalswift I'm not agains that annotation, I just don't understand why it's added to GetCoin.

Empact commented at 2:49 PM on November 22, 2018: member

I'm into adding NODISCARD to GetKey and GetValue as well, for coherence between the test and source changes.

Here's the patch for that:

diff --git a/src/dbwrapper.h b/src/dbwrapper.h
index 416f5e839..9460e5ff4 100644
--- a/src/dbwrapper.h
+++ b/src/dbwrapper.h
@@ -5,6 +5,7 @@
 #ifndef BITCOIN_DBWRAPPER_H
 #define BITCOIN_DBWRAPPER_H
 
+#include <attributes.h>
 #include <clientversion.h>
 #include <fs.h>
 #include <serialize.h>
@@ -144,7 +145,8 @@ public:
 
     void Next();
 
-    template<typename K> bool GetKey(K& key) {
+    template<typename K>
+    NODISCARD bool GetKey(K& key) {
         leveldb::Slice slKey = piter->key();
         try {
             CDataStream ssKey(slKey.data(), slKey.data() + slKey.size(), SER_DISK, CLIENT_VERSION);
@@ -155,7 +157,8 @@ public:
         return true;
     }
 
-    template<typename V> bool GetValue(V& value) {
+    template<typename V>
+    NODISCARD bool GetValue(V& value) {
         leveldb::Slice slValue = piter->value();
         try {
             CDataStream ssValue(slValue.data(), slValue.data() + slValue.size(), SER_DISK, CLIENT_VERSION);
diff --git a/src/txdb.cpp b/src/txdb.cpp
index 8447352c5..b91931e56 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -178,8 +178,9 @@ CCoinsViewCursor *CCoinsViewDB::Cursor() const
     // Cache key of first record
     if (i->pcursor->Valid()) {
         CoinEntry entry(&i->keyTmp.second);
-        i->pcursor->GetKey(entry);
-        i->keyTmp.first = entry.key;
+        if (i->pcursor->GetKey(entry)) {
+            i->keyTmp.first = entry.key;
+        }
     } else {
         i->keyTmp.first = 0; // Make sure Valid() and GetKey() return false
     }

MarcoFalke referenced this in commit 2a97f192ea on Nov 22, 2018

MarcoFalke merged this on Nov 22, 2018

MarcoFalke closed this on Nov 22, 2018

MarcoFalke deleted the branch on Nov 22, 2018

deadalnix referenced this in commit ff322359f4 on Sep 1, 2020

5tefan referenced this in commit 6255ba82f3 on Jul 28, 2021

5tefan referenced this in commit 7dd526fc83 on Jul 28, 2021

PastaPastaPasta referenced this in commit 1a29d78829 on Jul 29, 2021

Munkybooty referenced this in commit 9ff119694d on Aug 3, 2021

MarcoFalke locked this on Sep 8, 2021