test: Add BOOST_REQUIRE to getters returning optional #14771
Conversation
|
Do you mean to tag
|
fa0daa9 to
fa21ca0
Compare
|
utACK, I like the idea of adding |
|
utACK – |
|
|
||
| public: | ||
| bool GetCoin(const COutPoint& outpoint, Coin& coin) const override | ||
| NODISCARD bool GetCoin(const COutPoint& outpoint, Coin& coin) const override |
There was a problem hiding this comment.
Sorry, but this seems like a random change, why is this necessary?
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
@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? :-)
There was a problem hiding this comment.
@practicalswift I'm not agains that annotation, I just don't understand why it's added to GetCoin.
|
I'm into adding 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
} |
…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
Summary: bitcoin/bitcoin@fa21ca0 --- Backport of Core [[bitcoin/bitcoin#14771 | PR14771]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7278
…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.
…ional (#4297) * Merge bitcoin#14771: test: Add BOOST_REQUIRE to getters returning optional 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. * Ports [[nodiscard]] portion of bitcoin#14771
Usually the returned value is already checked for equality, but for sanity we might as well require that the getter successfully returned.