Merge pull request #8354 from Yannic/refactor_status

[C++] Refactor util::Status

conformance/conformance_cpp.cc

@@ -134,7 +134,7 @@ void DoTest(const ConformanceRequest& request, ConformanceResponse* response) {
                              &proto_binary, options);
       if (!status.ok()) {
         response->set_parse_error(string("Parse error: ") +
-                                  std::string(status.error_message()));
+                                  std::string(status.message()));
         return;
       }
 
@@ -189,7 +189,7 @@ void DoTest(const ConformanceRequest& request, ConformanceResponse* response) {
       if (!status.ok()) {
         response->set_serialize_error(
             string("Failed to serialize JSON output: ") +
-            std::string(status.error_message()));
+            std::string(status.message()));
         return;
       }
       break;

src/google/protobuf/stubs/logging.h

@@ -33,6 +33,7 @@
 
 #include <google/protobuf/stubs/macros.h>
 #include <google/protobuf/stubs/port.h>
+#include <google/protobuf/stubs/status.h>
 
 #include <google/protobuf/port_def.inc>
 
@@ -64,9 +65,6 @@ enum LogLevel {
 };
 
 class StringPiece;
-namespace util {
-class Status;
-}
 class uint128;
 namespace internal {
 

src/google/protobuf/stubs/status.cc

@@ -37,61 +37,64 @@
 namespace google {
 namespace protobuf {
 namespace util {
-namespace error {
-inline std::string CodeEnumToString(error::Code code) {
+namespace status_internal {
+namespace {
+
+inline std::string StatusCodeToString(StatusCode code) {
   switch (code) {
-    case OK:
+    case StatusCode::kOk:
       return "OK";
-    case CANCELLED:
+    case StatusCode::kCancelled:
       return "CANCELLED";
-    case UNKNOWN:
+    case StatusCode::kUnknown:
       return "UNKNOWN";
-    case INVALID_ARGUMENT:
+    case StatusCode::kInvalidArgument:
       return "INVALID_ARGUMENT";
-    case DEADLINE_EXCEEDED:
+    case StatusCode::kDeadlineExceeded:
       return "DEADLINE_EXCEEDED";
-    case NOT_FOUND:
+    case StatusCode::kNotFound:
       return "NOT_FOUND";
-    case ALREADY_EXISTS:
+    case StatusCode::kAlreadyExists:
       return "ALREADY_EXISTS";
-    case PERMISSION_DENIED:
+    case StatusCode::kPermissionDenied:
       return "PERMISSION_DENIED";
-    case UNAUTHENTICATED:
+    case StatusCode::kUnauthenticated:
       return "UNAUTHENTICATED";
-    case RESOURCE_EXHAUSTED:
+    case StatusCode::kResourceExhausted:
       return "RESOURCE_EXHAUSTED";
-    case FAILED_PRECONDITION:
+    case StatusCode::kFailedPrecondition:
       return "FAILED_PRECONDITION";
-    case ABORTED:
+    case StatusCode::kAborted:
       return "ABORTED";
-    case OUT_OF_RANGE:
+    case StatusCode::kOutOfRange:
       return "OUT_OF_RANGE";
-    case UNIMPLEMENTED:
+    case StatusCode::kUnimplemented:
       return "UNIMPLEMENTED";
-    case INTERNAL:
+    case StatusCode::kInternal:
       return "INTERNAL";
-    case UNAVAILABLE:
+    case StatusCode::kUnavailable:
       return "UNAVAILABLE";
-    case DATA_LOSS:
+    case StatusCode::kDataLoss:
       return "DATA_LOSS";
   }
 
   // No default clause, clang will abort if a code is missing from
   // above switch.
   return "UNKNOWN";
 }
-}  // namespace error.
+
+}  // namespace
 
 const Status Status::OK = Status();
-const Status Status::CANCELLED = Status(error::CANCELLED, "");
-const Status Status::UNKNOWN = Status(error::UNKNOWN, "");
+const Status Status::CANCELLED = Status(StatusCode::kCancelled, "");
+const Status Status::UNKNOWN = Status(StatusCode::kUnknown, "");
 
-Status::Status() : error_code_(error::OK) {
+Status::Status() : error_code_(StatusCode::kOk) {
 }
 
-Status::Status(error::Code error_code, StringPiece error_message)
+Status::Status(StatusCode error_code, StringPiece error_message)
     : error_code_(error_code) {
-  if (error_code != error::OK) {
+  if (error_code != StatusCode::kOk) {
     error_message_ = error_message.ToString();
   }
 }
@@ -112,13 +115,13 @@ bool Status::operator==(const Status& x) const {
 }
 
 std::string Status::ToString() const {
-  if (error_code_ == error::OK) {
+  if (error_code_ == StatusCode::kOk) {
     return "OK";
   } else {
     if (error_message_.empty()) {
-      return error::CodeEnumToString(error_code_);
+      return StatusCodeToString(error_code_);
     } else {
-      return error::CodeEnumToString(error_code_) + ":" +
+      return StatusCodeToString(error_code_) + ":" +
           error_message_;
     }
   }
@@ -129,6 +132,7 @@ std::ostream& operator<<(std::ostream& os, const Status& x) {
   return os;
 }
 
+}  // namespace status_internal
 }  // namespace util
 }  // namespace protobuf
 }  // namespace google

src/google/protobuf/stubs/status.h

@@ -27,42 +27,41 @@
 // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
 #ifndef GOOGLE_PROTOBUF_STUBS_STATUS_H_
 #define GOOGLE_PROTOBUF_STUBS_STATUS_H_
 
-#include <iosfwd>
 #include <string>
 
-#include <google/protobuf/stubs/common.h>
 #include <google/protobuf/stubs/stringpiece.h>
 
 #include <google/protobuf/port_def.inc>
 
 namespace google {
 namespace protobuf {
 namespace util {
-namespace error {
+namespace status_internal {
+
 // These values must match error codes defined in google/rpc/code.proto.
-enum Code {
-  OK = 0,
-  CANCELLED = 1,
-  UNKNOWN = 2,
-  INVALID_ARGUMENT = 3,
-  DEADLINE_EXCEEDED = 4,
-  NOT_FOUND = 5,
-  ALREADY_EXISTS = 6,
-  PERMISSION_DENIED = 7,
-  UNAUTHENTICATED = 16,
-  RESOURCE_EXHAUSTED = 8,
-  FAILED_PRECONDITION = 9,
-  ABORTED = 10,
-  OUT_OF_RANGE = 11,
-  UNIMPLEMENTED = 12,
-  INTERNAL = 13,
-  UNAVAILABLE = 14,
-  DATA_LOSS = 15,
+enum class StatusCode : int {
+  kOk = 0,
+  kCancelled = 1,
+  kUnknown = 2,
+  kInvalidArgument = 3,
+  kDeadlineExceeded = 4,
+  kNotFound = 5,
+  kAlreadyExists = 6,
+  kPermissionDenied = 7,
+  kUnauthenticated = 16,
+  kResourceExhausted = 8,
+  kFailedPrecondition = 9,
+  kAborted = 10,
+  kOutOfRange = 11,
+  kUnimplemented = 12,
+  kInternal = 13,
+  kUnavailable = 14,
+  kDataLoss = 15,
 };
-}  // namespace error
 
 class PROTOBUF_EXPORT Status {
  public:
@@ -71,9 +70,9 @@ class PROTOBUF_EXPORT Status {
 
   // Create a status in the canonical error space with the specified
   // code, and error message.  If "code == 0", error_message is
-  // ignored and a Status object identical to Status::OK is
+  // ignored and a Status object identical to Status::kOk is
   // constructed.
-  Status(error::Code error_code, StringPiece error_message);
+  Status(StatusCode error_code, StringPiece error_message);
   Status(const Status&);
   Status& operator=(const Status& x);
   ~Status() {}
@@ -85,17 +84,11 @@ class PROTOBUF_EXPORT Status {
 
   // Accessor
   bool ok() const {
-    return error_code_ == error::OK;
+    return error_code_ == StatusCode::kOk;
   }
-  int error_code() const {
+  StatusCode code() const {
     return error_code_;
   }
-  error::Code code() const {
-    return error_code_;
-  }
-  StringPiece error_message() const {
-    return error_message_;
-  }
   StringPiece message() const {
     return error_message_;
   }
@@ -109,13 +102,29 @@ class PROTOBUF_EXPORT Status {
   std::string ToString() const;
 
  private:
-  error::Code error_code_;
+  StatusCode error_code_;
   std::string error_message_;
 };
 
 // Prints a human-readable representation of 'x' to 'os'.
 PROTOBUF_EXPORT std::ostream& operator<<(std::ostream& os, const Status& x);
 
+}  // namespace status_internal
+
+using ::google::protobuf::util::status_internal::Status;
+using ::google::protobuf::util::status_internal::StatusCode;
+
+namespace error {
+
+// TODO(yannic): Remove these.
+constexpr StatusCode OK = StatusCode::kOk;
+constexpr StatusCode CANCELLED = StatusCode::kCancelled;
+constexpr StatusCode UNKNOWN = StatusCode::kUnknown;
+constexpr StatusCode INVALID_ARGUMENT = StatusCode::kInvalidArgument;
+constexpr StatusCode NOT_FOUND = StatusCode::kNotFound;
+constexpr StatusCode INTERNAL = StatusCode::kInternal;
+
+}  // namespace error
 }  // namespace util
 }  // namespace protobuf
 }  // namespace google

src/google/protobuf/stubs/status_test.cc

@@ -39,17 +39,13 @@ namespace protobuf {
 namespace {
 TEST(Status, Empty) {
   util::Status status;
-  EXPECT_EQ(util::error::OK, util::Status::OK.error_code());
   EXPECT_EQ(util::error::OK, util::Status::OK.code());
   EXPECT_EQ("OK", util::Status::OK.ToString());
 }
 
 TEST(Status, GenericCodes) {
-  EXPECT_EQ(util::error::OK, util::Status::OK.error_code());
   EXPECT_EQ(util::error::OK, util::Status::OK.code());
-  EXPECT_EQ(util::error::CANCELLED, util::Status::CANCELLED.error_code());
   EXPECT_EQ(util::error::CANCELLED, util::Status::CANCELLED.code());
-  EXPECT_EQ(util::error::UNKNOWN, util::Status::UNKNOWN.error_code());
   EXPECT_EQ(util::error::UNKNOWN, util::Status::UNKNOWN.code());
 }
 
@@ -69,17 +65,17 @@ TEST(Status, CheckOK) {
 TEST(Status, ErrorMessage) {
   util::Status status(util::error::INVALID_ARGUMENT, "");
   EXPECT_FALSE(status.ok());
-  EXPECT_EQ("", status.error_message().ToString());
+  EXPECT_EQ("", status.message().ToString());
   EXPECT_EQ("", status.message().ToString());
   EXPECT_EQ("INVALID_ARGUMENT", status.ToString());
   status = util::Status(util::error::INVALID_ARGUMENT, "msg");
   EXPECT_FALSE(status.ok());
-  EXPECT_EQ("msg", status.error_message().ToString());
+  EXPECT_EQ("msg", status.message().ToString());
   EXPECT_EQ("msg", status.message().ToString());
   EXPECT_EQ("INVALID_ARGUMENT:msg", status.ToString());
   status = util::Status(util::error::OK, "msg");
   EXPECT_TRUE(status.ok());
-  EXPECT_EQ("", status.error_message().ToString());
+  EXPECT_EQ("", status.message().ToString());
   EXPECT_EQ("", status.message().ToString());
   EXPECT_EQ("OK", status.ToString());
 }

src/google/protobuf/stubs/statusor.h

@@ -32,7 +32,7 @@
 // object. StatusOr models the concept of an object that is either a
 // usable value, or an error Status explaining why such a value is
 // not present. To this end, StatusOr<T> does not allow its Status
-// value to be Status::OK. Further, StatusOr<T*> does not allow the
+// value to be Status::kOk. Further, StatusOr<T*> does not allow the
 // contained pointer to be nullptr.
 //
 // The primary use-case for StatusOr<T> is as the return value of a
@@ -110,8 +110,8 @@ class StatusOr {
   // value, so it is convenient and sensible to be able to do 'return
   // Status()' when the return type is StatusOr<T>.
   //
-  // REQUIRES: status != Status::OK. This requirement is DCHECKed.
-  // In optimized builds, passing Status::OK here will have the effect
+  // REQUIRES: status != Status::kOk. This requirement is DCHECKed.
+  // In optimized builds, passing Status::kOk here will have the effect
   // of passing PosixErrorSpace::EINVAL as a fallback.
   StatusOr(const Status& status);  // NOLINT
 
@@ -143,7 +143,7 @@ class StatusOr {
   StatusOr& operator=(const StatusOr<U>& other);
 
   // Returns a reference to our status. If this contains a T, then
-  // returns Status::OK.
+  // returns Status::kOk.
   const Status& status() const;
 
   // Returns this->status().ok()
@@ -196,7 +196,8 @@ inline StatusOr<T>::StatusOr()
 template<typename T>
 inline StatusOr<T>::StatusOr(const Status& status) {
   if (status.ok()) {
-    status_ = Status(error::INTERNAL, "Status::OK is not a valid argument.");
+    status_ =
+        Status(StatusCode::kInternal, "Status::kOk is not a valid argument.");
   } else {
     status_ = status;
   }
@@ -205,7 +206,7 @@ inline StatusOr<T>::StatusOr(const Status& status) {
 template<typename T>
 inline StatusOr<T>::StatusOr(const T& value) {
   if (internal::StatusOrHelper::Specialize<T>::IsValueNull(value)) {
-    status_ = Status(error::INTERNAL, "nullptr is not a valid argument.");
+    status_ = Status(StatusCode::kInternal, "nullptr is not a valid argument.");
   } else {
     status_ = Status::OK;
     value_ = value;

src/google/protobuf/util/internal/datapiece.cc

@@ -48,7 +48,6 @@ namespace util {
 namespace converter {
 
 using util::Status;
-using util::error::Code;
 
 namespace {
 

src/google/protobuf/util/internal/json_stream_parser_test.cc

@@ -139,7 +139,7 @@ class JsonStreamParserTest : public ::testing::Test {
       }) {
     util::Status result = RunTest(json, split, setup);
     EXPECT_EQ(util::error::INVALID_ARGUMENT, result.code());
-    StringPiece error_message(result.error_message());
+    StringPiece error_message(result.message());
     EXPECT_EQ(error_prefix, error_message.substr(0, error_prefix.size()));
   }
 
@@ -150,7 +150,7 @@ class JsonStreamParserTest : public ::testing::Test {
       }) {
     util::Status result = RunTest(json, split, setup);
     EXPECT_EQ(util::error::INVALID_ARGUMENT, result.code());
-    StringPiece error_message(result.error_message());
+    StringPiece error_message(result.message());
     EXPECT_EQ(error_prefix, error_message.substr(0, error_prefix.size()));
   }