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
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -2,17 +2,18 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <attributes.h>
#include <coins.h>
#include <consensus/validation.h>
#include <script/standard.h>
#include <test/test_bitcoin.h>
#include <uint256.h>
#include <undo.h>
#include <util/strencodings.h>
#include <test/test_bitcoin.h>
#include <validation.h>
#include <consensus/validation.h>

#include <vector>
#include <map>
#include <vector>

#include <boost/test/unit_test.hpp>

@@ -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.

{
std::map<COutPoint, Coin>::const_iterator it = map_.find(outpoint);
if (it == map_.end()) {
@@ -102,15 +102,15 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
char key_res;
uint256 val_res;

it->GetKey(key_res);
it->GetValue(val_res);
BOOST_REQUIRE(it->GetKey(key_res));
BOOST_REQUIRE(it->GetValue(val_res));
BOOST_CHECK_EQUAL(key_res, key);
BOOST_CHECK_EQUAL(val_res.ToString(), in.ToString());

it->Next();

it->GetKey(key_res);
it->GetValue(val_res);
BOOST_REQUIRE(it->GetKey(key_res));
BOOST_REQUIRE(it->GetValue(val_res));
BOOST_CHECK_EQUAL(key_res, key2);
BOOST_CHECK_EQUAL(val_res.ToString(), in2.ToString());

@@ -187,7 +187,7 @@ struct TestArgsManager : public ArgsManager
m_config_args.clear();
}
std::string error;
ReadConfigStream(streamConfig, error);
BOOST_REQUIRE(ReadConfigStream(streamConfig, error));
}
void SetNetworkOnlyArg(const std::string arg)
{
@@ -150,7 +150,7 @@ class ArgsManager
std::set<std::string> m_network_only_args GUARDED_BY(cs_args);
std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);

bool ReadConfigStream(std::istream& stream, std::string& error, bool ignore_invalid_keys = false);
NODISCARD bool ReadConfigStream(std::istream& stream, std::string& error, bool ignore_invalid_keys = false);

public:
ArgsManager();
@@ -161,7 +161,7 @@ class ArgsManager
void SelectConfigNetwork(const std::string& network);

NODISCARD bool ParseParameters(int argc, const char* const argv[], std::string& error);
bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false);
NODISCARD bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false);

/**
* Log warnings for options in m_section_only_args when
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.