Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Add BOOST_REQUIRE to getters returning optional #14771

Merged
merged 1 commit into from Nov 22, 2018

Conversation

Projects
None yet
5 participants
@MarcoFalke
Copy link
Member

commented Nov 20, 2018

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

@MarcoFalke MarcoFalke added the Tests label Nov 20, 2018

@Empact

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

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

GetKey has some unchecked uses in the codebase.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1811-testNoDiscard branch to fa21ca0 Nov 21, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

utACK – NODISCARD is great!

@@ -36,7 +37,7 @@ class CCoinsViewTest : public CCoinsView
std::map<COutPoint, Coin> map_;

public:
bool GetCoin(const COutPoint& outpoint, Coin& coin) const override
NODISCARD bool GetCoin(const COutPoint& outpoint, Coin& coin) const override

This comment has been minimized.

Copy link
@promag

promag Nov 22, 2018

Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 22, 2018

Member

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 :-)

This comment has been minimized.

Copy link
@promag

promag Nov 22, 2018

Member

There are tons of that?

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 22, 2018

Member

@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? :-)

This comment has been minimized.

Copy link
@promag

promag Nov 22, 2018

Member

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

@Empact

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

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 added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 22, 2018

Merge bitcoin#14771: test: Add BOOST_REQUIRE to getters returning opt…
…ional

fa21ca0 test: Add BOOST_REQUIRE to getters returning optional (MarcoFalke)

Pull request description:

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

Tree-SHA512: 0d613a9a721c61bd7a115ebc681a0890df09b8e5775f176ac18b3a586f2ca57bee0b5b816f5a7c314ff3ac6cbb2a4d9c434f8459e054a7c8a6934a75f0120c2a

@MarcoFalke MarcoFalke merged commit fa21ca0 into bitcoin:master Nov 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1811-testNoDiscard branch Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.