From 610e112d168cb99b782ca18986fd25cd72ca5335 Mon Sep 17 00:00:00 2001 From: KongQun Yang Date: Mon, 22 Jan 2018 16:09:14 -0800 Subject: [PATCH] Cleans up Status class - Removed SetError, Swap, Matches functions, which are not used - Added moveable version of Update function - Added VLOG(1) warning in Status for non-ok status - Replaced OS_WIN with _WIN32 as OS_WIN may not have been defined. Change-Id: Ib6b7aab6e6fee270937b150f1e4bf993e914a568 --- packager/media/demuxer/demuxer.cc | 6 +-- packager/status.cc | 14 +++++++ packager/status.h | 49 +++---------------------- packager/status_unittest.cc | 61 ++++--------------------------- 4 files changed, 29 insertions(+), 101 deletions(-) diff --git a/packager/media/demuxer/demuxer.cc b/packager/media/demuxer/demuxer.cc index 82414a4fed..7b4cc1ee7f 100644 --- a/packager/media/demuxer/demuxer.cc +++ b/packager/media/demuxer/demuxer.cc @@ -269,9 +269,9 @@ void Demuxer::ParserInitEvent( stream_info->set_language(iter->second); } if (stream_info->is_encrypted()) { - init_event_status_.SetError( - error::INVALID_ARGUMENT, - "A decryption key source is not provided for an encrypted stream."); + init_event_status_.Update(Status(error::INVALID_ARGUMENT, + "A decryption key source is not " + "provided for an encrypted stream.")); } else { init_event_status_.Update( DispatchStreamInfo(stream_index, stream_info)); diff --git a/packager/status.cc b/packager/status.cc index b866e7ab36..fd6b1d5915 100644 --- a/packager/status.cc +++ b/packager/status.cc @@ -64,6 +64,20 @@ std::string ErrorCodeToString(Code error_code) { const Status Status::OK = Status(error::OK, ""); const Status Status::UNKNOWN = Status(error::UNKNOWN, ""); +Status::Status(error::Code error_code, const std::string& error_message) + : error_code_(error_code) { + if (!ok()) { + error_message_ = error_message; + if (!error_message.empty()) + VLOG(1) << ToString(); + } +} + +void Status::Update(Status new_status) { + if (ok()) + *this = std::move(new_status); +} + std::string Status::ToString() const { if (error_code_ == error::OK) return "OK"; diff --git a/packager/status.h b/packager/status.h index 28b3066890..786a54ccf0 100644 --- a/packager/status.h +++ b/packager/status.h @@ -11,7 +11,7 @@ #include #if defined(SHARED_LIBRARY_BUILD) -#if defined(OS_WIN) +#if defined(_WIN32) #if defined(SHAKA_IMPLEMENTATION) #define SHAKA_EXPORT __declspec(dllexport) @@ -19,7 +19,7 @@ #define SHAKA_EXPORT __declspec(dllimport) #endif // defined(SHAKA_IMPLEMENTATION) -#else // defined(OS_WIN) +#else // defined(_WIN32) #if defined(SHAKA_IMPLEMENTATION) #define SHAKA_EXPORT __attribute__((visibility("default"))) @@ -27,7 +27,7 @@ #define SHAKA_EXPORT #endif -#endif // defined(OS_WIN) +#endif // defined(_WIN32) #else // defined(SHARED_LIBRARY_BUILD) #define SHAKA_EXPORT @@ -115,13 +115,7 @@ class SHAKA_EXPORT Status { /// Create a status with the specified code, and error message. /// If "error_code == error::OK", error_message is ignored and a Status /// object identical to Status::OK is constructed. - Status(error::Code error_code, const std::string& error_message) - : error_code_(error_code) { - if (!ok()) - error_message_ = error_message; - } - - ~Status() {} + Status(error::Code error_code, const std::string& error_message); /// @name Some pre-defined Status objects. /// @{ @@ -129,18 +123,6 @@ class SHAKA_EXPORT Status { static const Status UNKNOWN; /// @} - /// Store the specified error in this Status object. - /// If "error_code == error::OK", error_message is ignored and a Status - /// object identical to Status::OK is constructed. - void SetError(error::Code error_code, const std::string& error_message) { - if (error_code == error::OK) { - Clear(); - } else { - error_code_ = error_code; - error_message_ = error_message; - } - } - /// If "ok()", stores "new_status" into *this. If "!ok()", preserves /// the current "error_code()/error_message()", /// @@ -149,16 +131,7 @@ class SHAKA_EXPORT Status { /// if (overall_status.ok()) overall_status = new_status /// Use: /// overall_status.Update(new_status); - void Update(const Status& new_status) { - if (ok()) - *this = new_status; - } - - /// Clear this status object to contain the OK code and no error message. - void Clear() { - error_code_ = error::OK; - error_message_ = ""; - } + void Update(Status new_status); bool ok() const { return error_code_ == error::OK; } error::Code error_code() const { return error_code_; } @@ -169,21 +142,9 @@ class SHAKA_EXPORT Status { } bool operator!=(const Status& x) const { return !(*this == x); } - /// @return true iff this has the same error_code as "x", i.e., the two - /// Status objects are identical except possibly for the error - /// message. - bool Matches(const Status& x) const { return error_code_ == x.error_code(); } - /// @return A combination of the error code name and message. std::string ToString() const; - void Swap(Status* other) { - error::Code error_code = error_code_; - error_code_ = other->error_code_; - other->error_code_ = error_code; - error_message_.swap(other->error_message_); - } - private: error::Code error_code_; std::string error_message_; diff --git a/packager/status_unittest.cc b/packager/status_unittest.cc index 4acc8f9729..723e259073 100644 --- a/packager/status_unittest.cc +++ b/packager/status_unittest.cc @@ -40,18 +40,6 @@ TEST(Status, ConstructorOK) { CheckStatus(Status(error::OK, "msg"), error::OK, ""); } -TEST(Status, SetError) { - Status status; - status.SetError(error::CANCELLED, "message"); - CheckStatus(status, error::CANCELLED, "message"); -} - -TEST(Status, SetErrorOK) { - Status status(error::CANCELLED, "message"); - status.SetError(error::OK, "msg"); - CheckStatus(status, error::OK, ""); -} - TEST(Status, Unknown) { CheckStatus(Status::UNKNOWN, error::UNKNOWN, ""); } @@ -60,12 +48,6 @@ TEST(Status, Filled) { CheckStatus(Status(error::CANCELLED, "message"), error::CANCELLED, "message"); } -TEST(Status, Clear) { - Status status(error::CANCELLED, "message"); - status.Clear(); - CheckStatus(status, error::OK, ""); -} - TEST(Status, Copy) { Status a(error::CANCELLED, "message"); Status b(a); @@ -92,7 +74,7 @@ TEST(Status, Update) { Status s; s.Update(Status::OK); ASSERT_TRUE(s.ok()); - Status a(error::CANCELLED, "message"); + const Status a(error::CANCELLED, "message"); s.Update(a); ASSERT_EQ(s, a); Status b(error::UNIMPLEMENTED, "other message"); @@ -103,41 +85,12 @@ TEST(Status, Update) { ASSERT_FALSE(s.ok()); } -TEST(Status, Swap) { - Status a(error::CANCELLED, "message"); - Status b = a; - Status c; - c.Swap(&a); - ASSERT_EQ(c, b); - ASSERT_EQ(a, Status::OK); -} - -TEST(Status, MatchOK) { - ASSERT_TRUE(Status().Matches(Status::OK)); -} - -TEST(Status, MatchSame) { - Status a(error::UNKNOWN, "message"); - Status b(error::UNKNOWN, "message"); - ASSERT_TRUE(a.Matches(b)); -} - -TEST(Status, MatchCopy) { - Status a(error::UNKNOWN, "message"); - Status b = a; - ASSERT_TRUE(a.Matches(b)); -} - -TEST(Status, MatchDifferentCode) { - Status a(error::UNKNOWN, "message"); - Status b(error::CANCELLED, "message"); - ASSERT_TRUE(!a.Matches(b)); -} - -TEST(Status, MatchDifferentMessage) { - Status a(error::UNKNOWN, "message"); - Status b(error::UNKNOWN, "another"); - ASSERT_TRUE(a.Matches(b)); +// Will trigger copy ellision. +TEST(Status, Update2) { + Status s; + ASSERT_TRUE(s.ok()); + s.Update(Status(error::INVALID_ARGUMENT, "some message")); + ASSERT_EQ(error::INVALID_ARGUMENT, s.error_code()); } TEST(Status, EqualsOK) {