From e3f5a887625b3af1b7689248b45e940b629948f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 19 Feb 2019 12:50:57 +0100 Subject: [PATCH] Forbid ambiguous casts to and from ResultT (#8147) * Forbid ambiguous casts to and from ResultT * Reformat * Changed enabled_if checks to check for implicit casts to Result * Added comments --- arangod/Cluster/ResultT.h | 61 ++++++++++++++++++++--- arangod/RestHandler/RestRepairHandler.cpp | 16 +++--- 2 files changed, 63 insertions(+), 14 deletions(-) diff --git a/arangod/Cluster/ResultT.h b/arangod/Cluster/ResultT.h index 878da1fe31..5b7a6f6377 100644 --- a/arangod/Cluster/ResultT.h +++ b/arangod/Cluster/ResultT.h @@ -23,6 +23,8 @@ #ifndef ARANGODB_BASICS_RESULT_T_H #define ARANGODB_BASICS_RESULT_T_H +#include + #include #include "Basics/Common.h" @@ -55,6 +57,7 @@ namespace arangodb { // or, having a `Result result` with result.fail() being true, // return result; // . +// Some implicit conversions are disabled when they could cause ambiguity. template class ResultT { public: @@ -74,22 +77,54 @@ class ResultT { return ResultT(boost::none, errorNumber, errorMessage); } - // These are not explicit on purpose + ResultT static error(Result const& other) { + TRI_ASSERT(other.fail()); + return ResultT(boost::none, other); + } + + ResultT static error(Result&& other) { + TRI_ASSERT(other.fail()); + return ResultT(boost::none, std::move(other)); + } + + // This is disabled if U is implicitly convertible to Result + // (e.g., if U = int) to avoid ambiguous construction. + // Use ::success() or ::error() instead in that case. + template ::value>> + // This is not explicit on purpose + // NOLINTNEXTLINE(google-explicit-constructor) ResultT(Result const& other) : _result(other) { // .ok() is not allowed here, as _val should be expected to be initialized // iff .ok() is true. TRI_ASSERT(other.fail()); } + // This is disabled if U is implicitly convertible to Result + // (e.g., if U = int) to avoid ambiguous construction. + // Use ::success() or ::error() instead in that case. + template ::value>> + // This is not explicit on purpose + // NOLINTNEXTLINE(google-explicit-constructor) ResultT(Result&& other) : _result(std::move(other)) { // .ok() is not allowed here, as _val should be expected to be initialized // iff .ok() is true. TRI_ASSERT(other.fail()); } - // These are not explicit on purpose + // This is disabled if U is implicitly convertible to Result + // (e.g., if U = int) to avoid ambiguous construction. + // Use ::success() or ::error() instead in that case. + template ::value>> + // This is not explicit on purpose + // NOLINTNEXTLINE(google-explicit-constructor) ResultT(T&& val) : ResultT(std::move(val), TRI_ERROR_NO_ERROR) {} + // This is disabled if U is implicitly convertible to Result + // (e.g., if U = int) to avoid ambiguous construction. + // Use ::success() or ::error() instead in that case. + template ::value>> + // This is not explicit on purpose + // NOLINTNEXTLINE(google-explicit-constructor) ResultT(T const& val) : ResultT(val, TRI_ERROR_NO_ERROR) {} ResultT() = delete; @@ -104,8 +139,6 @@ class ResultT { return *this; } - Result copy_result() const { return *this; } - // These would be very convenient, but also make it very easy to accidentally // use the value of an error-result. So don't add them. // @@ -124,7 +157,17 @@ class ResultT { T const&& operator*() const&& { return get(); } - explicit operator bool() const { return ok(); } + // Allow convenient check to allow for code like + // if (res) { /* use res.get() */ } else { /* handle error res.result() */ } + // . Is disabled for bools to avoid accidental confusion of + // if (res) + // with + // if (res.get()) + // . + template ::value>> + explicit operator bool() const { + return ok(); + } T const& get() const { return _val.get(); } @@ -163,7 +206,7 @@ class ResultT { // forwarded methods bool ok() const { return _result.ok(); } bool fail() const { return _result.fail(); } - bool is(uint64_t code) { return _result.is(code); } + bool is(int code) { return _result.is(code); } int errorNumber() const { return _result.errorNumber(); } std::string errorMessage() const { return _result.errorMessage(); } @@ -186,6 +229,12 @@ class ResultT { ResultT(boost::optional const& val_, int errorNumber, std::string const& errorMessage) : _result(errorNumber, errorMessage), _val(val_) {} + + ResultT(boost::optional&& val_, Result result) + : _result(std::move(result)), _val(std::move(val_)) {} + + ResultT(boost::optional&& val_, Result&& result) + : _result(std::move(result)), _val(std::move(val_)) {} }; } // namespace arangodb diff --git a/arangod/RestHandler/RestRepairHandler.cpp b/arangod/RestHandler/RestRepairHandler.cpp index 5ed5f3e436..8277c846ca 100644 --- a/arangod/RestHandler/RestRepairHandler.cpp +++ b/arangod/RestHandler/RestRepairHandler.cpp @@ -290,7 +290,7 @@ ResultT RestRepairHandler::jobFinished(std::string const& jobId) { break; case JobStatus::finished: - return true; + return ResultT::success(true); break; case JobStatus::failed: @@ -298,14 +298,14 @@ ResultT RestRepairHandler::jobFinished(std::string const& jobId) { << "RestRepairHandler::jobFinished: " << "Job " << jobId << " failed, aborting"; - return Result(TRI_ERROR_CLUSTER_REPAIRS_JOB_FAILED); + return ResultT::error(TRI_ERROR_CLUSTER_REPAIRS_JOB_FAILED); case JobStatus::missing: LOG_TOPIC(ERR, arangodb::Logger::CLUSTER) << "RestRepairHandler::jobFinished: " << "Job " << jobId << " went missing, aborting"; - return Result(TRI_ERROR_CLUSTER_REPAIRS_JOB_DISAPPEARED); + return ResultT::error(TRI_ERROR_CLUSTER_REPAIRS_JOB_DISAPPEARED); } } else { @@ -314,10 +314,10 @@ ResultT RestRepairHandler::jobFinished(std::string const& jobId) { << "Failed to get job status: " << "[" << jobStatus.errorNumber() << "] " << jobStatus.errorMessage(); - return std::move(std::move(jobStatus).result()); + return ResultT::error(std::move(jobStatus.result())); } - return false; + return ResultT::success(false); } Result RestRepairHandler::executeRepairOperations(DatabaseID const& databaseId, @@ -636,7 +636,7 @@ ResultT RestRepairHandler::checkReplicationFactor(DatabaseID const& databa << "No ClusterInfo instance"; generateError(rest::ResponseCode::SERVER_ERROR, TRI_ERROR_HTTP_SERVER_ERROR); - return Result(TRI_ERROR_INTERNAL); + return ResultT::error(TRI_ERROR_INTERNAL); } clusterInfo->loadPlan(); @@ -656,11 +656,11 @@ ResultT RestRepairHandler::checkReplicationFactor(DatabaseID const& databa << "replicationFactor is " << collection->replicationFactor() << ", but the shard has " << dbServers.size() << " DBServers."; - return false; + return ResultT::success(false); } } - return true; + return ResultT::success(true); } void RestRepairHandler::generateResult(rest::ResponseCode code,