Buffet: utility function tweaks Various tweakes to helper functions and classes, split off from a larger CL that required them. 1. Added Error::AddToPrintf to help add formatted error messages. 2. Added Error::GetFirstError to get the innermost error occurred. 3. Added string_utils::ToString and swept code using std::to_string in order to ensure we format doubles correctly (using %g instead of %f format specifier) and bool values (using "true"/"false" instead of 1/0). 4. Fixed C-style cast in http_utils.h and using static_cast now. 5. Fixed a few linter warnings. Also since the linter was updated there is no reason to have some NOLINT since many C++11 features are now recognized properly by cpplint. BUG=None TEST=All unit tests pass. Change-Id: I208ffaa3f0ec0a5ff78bf9e8151e784ec8cd77e2 Reviewed-on: https://chromium-review.googlesource.com/202962 Tested-by: Alex Vakulenko <avakulenko@chromium.org> Reviewed-by: Christopher Wiley <wiley@chromium.org> Commit-Queue: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/device_registration_info.cc b/buffet/device_registration_info.cc index 4aab715..5447726 100644 --- a/buffet/device_registration_info.cc +++ b/buffet/device_registration_info.cc
@@ -66,9 +66,7 @@ const std::string& access_token) { std::string authorization = buffet::string_utils::Join(' ', access_token_type, access_token); - // Linter doesn't like the ; after } on the following line... - return {buffet::http::request_header::kAuthorization, - authorization}; // NOLINT + return {buffet::http::request_header::kAuthorization, authorization}; } std::unique_ptr<base::DictionaryValue> ParseOAuthResponse( @@ -320,8 +318,8 @@ if (!param_value.empty()) return true; - Error::AddTo(error, kErrorDomainBuffet, "missing_parameter", - "Parameter " + param_name + " not specified"); + Error::AddToPrintf(error, kErrorDomainBuffet, "missing_parameter", + "Parameter %s not specified", param_name.c_str()); return false; } @@ -364,9 +362,9 @@ set_device_configuration_params->Append(param1); base::ListValue* vendor_commands = new base::ListValue; - for (auto&& pair : commands) { + for (const auto& pair : commands) { base::ListValue* params = new base::ListValue; - for (auto&& param_name : pair.second) { + for (const auto& param_name : pair.second) { base::DictionaryValue* param = new base::DictionaryValue; param->SetString("name", param_name); params->Append(param);
diff --git a/buffet/error.cc b/buffet/error.cc index bccb9b8..db5543d 100644 --- a/buffet/error.cc +++ b/buffet/error.cc
@@ -5,6 +5,7 @@ #include "buffet/error.h" #include <base/logging.h> +#include <base/strings/stringprintf.h> using buffet::Error; using buffet::ErrorPtr; @@ -36,6 +37,15 @@ } } +void Error::AddToPrintf(ErrorPtr* error, const std::string& domain, + const std::string& code, const char* format, ...) { + va_list ap; + va_start(ap, format); + std::string message = base::StringPrintV(format, ap); + va_end(ap); + AddTo(error, domain, code, message); +} + bool Error::HasDomain(const std::string& domain) const { const Error* err = this; while (err) { @@ -56,6 +66,13 @@ return false; } +const Error* Error::GetFirstError() const { + const Error* err = this; + while (err->GetInnerError()) + err = err->GetInnerError(); + return err; +} + Error::Error(const std::string& domain, const std::string& code, const std::string& message, ErrorPtr inner_error) : domain_(domain), code_(code), message_(message),
diff --git a/buffet/error.h b/buffet/error.h index 2f5ba66..4e90c2b 100644 --- a/buffet/error.h +++ b/buffet/error.h
@@ -30,6 +30,11 @@ // the error chain pointed to by |error|. static void AddTo(ErrorPtr* error, const std::string& domain, const std::string& code, const std::string& message); + // Same as the Error::AddTo above, but allows to pass in a printf-like + // format string and optional parameters to format the error message. + static void AddToPrintf(ErrorPtr* error, const std::string& domain, + const std::string& code, + const char* format, ...) PRINTF_FORMAT(4, 5); // Returns the error domain, code and message const std::string& GetDomain() const { return domain_; } @@ -46,6 +51,10 @@ // Gets a pointer to the inner error, if present. Returns nullptr otherwise. const Error* GetInnerError() const { return inner_error_.get(); } + // Gets a pointer to the first error occurred. + // Returns itself if no inner error are available. + const Error* GetFirstError() const; + protected: // Constructor is protected since this object is supposed to be // created via the Create factory methods.
diff --git a/buffet/http_connection_curl.cc b/buffet/http_connection_curl.cc index 0dae74c..ebdd3cc 100644 --- a/buffet/http_connection_curl.cc +++ b/buffet/http_connection_curl.cc
@@ -121,7 +121,7 @@ if (header_list) curl_slist_free_all(header_list); if (ret != CURLE_OK) { - Error::AddTo(error, http::curl::kErrorDomain, std::to_string(ret), + Error::AddTo(error, http::curl::kErrorDomain, string_utils::ToString(ret), curl_easy_strerror(ret)); } else { LOG(INFO) << "Response: " << GetResponseStatusCode() << " ("
diff --git a/buffet/http_connection_fake.cc b/buffet/http_connection_fake.cc index 87fbf23..383f6d4 100644 --- a/buffet/http_connection_fake.cc +++ b/buffet/http_connection_fake.cc
@@ -37,7 +37,7 @@ bool Connection::FinishRequest(ErrorPtr* error) { request_.AddHeaders({{request_header::kContentLength, - std::to_string(request_.GetData().size())}}); + string_utils::ToString(request_.GetData().size())}}); fake::Transport* transport = static_cast<fake::Transport*>(transport_.get()); CHECK(transport) << "Expecting a fake transport"; auto handler = transport->GetHandler(request_.GetURL(), request_.GetMethod());
diff --git a/buffet/http_request.cc b/buffet/http_request.cc index df09ca4..c08e797 100644 --- a/buffet/http_request.cc +++ b/buffet/http_request.cc
@@ -193,11 +193,11 @@ p.second != range_value_omitted) { std::string range; if (p.first != range_value_omitted) { - range = std::to_string(p.first); + range = string_utils::ToString(p.first); } range += '-'; if (p.second != range_value_omitted) { - range += std::to_string(p.second); + range += string_utils::ToString(p.second); } ranges.push_back(range); }
diff --git a/buffet/http_request.h b/buffet/http_request.h index 8aedc45..e5b9710 100644 --- a/buffet/http_request.h +++ b/buffet/http_request.h
@@ -5,6 +5,7 @@ #ifndef BUFFET_HTTP_REQUEST_H_ #define BUFFET_HTTP_REQUEST_H_ +#include <limits> #include <map> #include <memory> #include <string> @@ -303,7 +304,7 @@ // range_value_omitted is used in |ranges_| list to indicate omitted value. // E.g. range (10,range_value_omitted) represents bytes from 10 to the end // of the data stream. - const uint64_t range_value_omitted = (uint64_t)-1; + const uint64_t range_value_omitted = std::numeric_limits<uint64_t>::max(); DISALLOW_COPY_AND_ASSIGN(Request); };
diff --git a/buffet/http_transport_fake.cc b/buffet/http_transport_fake.cc index 0e7dfc5..471de11 100644 --- a/buffet/http_transport_fake.cc +++ b/buffet/http_transport_fake.cc
@@ -14,6 +14,7 @@ #include "buffet/http_connection_fake.h" #include "buffet/http_request.h" #include "buffet/mime_utils.h" +#include "buffet/string_utils.h" #include "buffet/url_utils.h" namespace buffet { @@ -128,7 +129,7 @@ } void ServerRequestResponseBase::AddHeaders(const HeaderList& headers) { - for (auto&& pair : headers) { + for (const auto& pair : headers) { if (pair.second.empty()) headers_.erase(pair.first); else @@ -170,7 +171,7 @@ status_code_ = status_code; AddData(data, data_size); AddHeaders({ - {response_header::kContentLength, std::to_string(data_size)}, + {response_header::kContentLength, string_utils::ToString(data_size)}, {response_header::kContentType, mime_type} }); } @@ -194,7 +195,7 @@ void ServerResponse::ReplyJson(int status_code, const http::FormFieldList& fields) { base::DictionaryValue json; - for (auto&& pair : fields) { + for (const auto& pair : fields) { json.SetString(pair.first, pair.second); } ReplyJson(status_code, &json); @@ -250,7 +251,7 @@ {505, "HTTP Version Not Supported"}, }; - for (auto&& pair : status_text_map) { + for (const auto& pair : status_text_map) { if (pair.first == status_code_) return pair.second; }
diff --git a/buffet/http_utils_unittest.cc b/buffet/http_utils_unittest.cc index fc86d31..404ef7b 100644 --- a/buffet/http_utils_unittest.cc +++ b/buffet/http_utils_unittest.cc
@@ -28,13 +28,13 @@ fake::ServerResponse* response) { response->Reply(status_code::Ok, request.GetData(), request.GetHeader(request_header::kContentType).c_str()); -}; +} // Returns the request method as a plain text response. static void EchoMethodHandler(const fake::ServerRequest& request, fake::ServerResponse* response) { response->ReplyText(status_code::Ok, request.GetMethod(), mime::text::kPlain); -}; +} /////////////////////////////////////////////////////////////////////////////// TEST(HttpUtils, SendRequest_BinaryData) { @@ -43,7 +43,7 @@ base::Bind(EchoDataHandler)); // Test binary data round-tripping. - std::vector<unsigned char> custom_data({0xFF, 0x00, 0x80, 0x40, 0xC0, 0x7F}); + std::vector<unsigned char> custom_data{0xFF, 0x00, 0x80, 0x40, 0xC0, 0x7F}; auto response = http::SendRequest(request_type::kPost, kEchoUrl, custom_data.data(), custom_data.size(), mime::application::kOctet_stream, @@ -114,7 +114,7 @@ base::DictionaryValue json; json.SetString("method", request.GetMethod()); json.SetString("data", request.GetDataAsString()); - for (auto&& pair : request.GetHeaders()) { + for (const auto& pair : request.GetHeaders()) { json.SetString("header." + pair.first, pair.second); } response->ReplyJson(status_code::Ok, &json); @@ -204,7 +204,7 @@ EXPECT_EQ("256", request.GetHeader(request_header::kContentLength)); EXPECT_EQ(mime::application::kOctet_stream, request.GetHeader(request_header::kContentType)); - auto&& data = request.GetData(); + const auto& data = request.GetData(); EXPECT_EQ(256, data.size()); // Sum up all the bytes. @@ -218,8 +218,7 @@ /// Fill the data buffer with bytes from 0x00 to 0xFF. std::vector<unsigned char> data(256); - unsigned char counter = 0xFF; - std::generate(data.begin(), data.end(), [&counter]() { return ++counter; }); + std::iota(data.begin(), data.end(), 0); auto response = http::PostBinary(kFakeUrl, data.data(), data.size(), transport, nullptr); @@ -311,7 +310,7 @@ transport->AddHandler(kFakeUrl, request_type::kPost, base::Bind(JsonHandler)); // Test valid JSON responses (with success or error codes). - for (auto&& item : {"200;data", "400;wrong", "500;Internal Server error"}) { + for (auto item : {"200;data", "400;wrong", "500;Internal Server error"}) { auto pair = string_utils::SplitAtFirst(item, ';'); auto response = http::PostFormData(kFakeUrl, { {"code", pair.first}, @@ -322,7 +321,7 @@ EXPECT_NE(nullptr, json.get()); std::string value; EXPECT_TRUE(json->GetString("data", &value)); - EXPECT_EQ(pair.first, std::to_string(code)); + EXPECT_EQ(pair.first, string_utils::ToString(code)); EXPECT_EQ(pair.second, value); }
diff --git a/buffet/string_utils.cc b/buffet/string_utils.cc index 55c7d3f..8916e96 100644 --- a/buffet/string_utils.cc +++ b/buffet/string_utils.cc
@@ -9,6 +9,7 @@ #include <utility> #include <base/strings/string_util.h> +#include <base/strings/stringprintf.h> namespace buffet { namespace string_utils { @@ -36,7 +37,7 @@ if (trim_whitespaces) { std::for_each(tokens.begin(), tokens.end(), - [](std::string& str) { // NOLINT(runtime/references) + [](std::string& str) { base::TrimWhitespaceASCII(str, base::TRIM_ALL, &str); }); } @@ -86,5 +87,13 @@ return str1 + delimiter + str2; } +std::string ToString(double value) { + return base::StringPrintf("%g", value); +} + +std::string ToString(bool value) { + return value ? "true" : "false"; +} + } // namespace string_utils } // namespace buffet
diff --git a/buffet/string_utils.h b/buffet/string_utils.h index 95da72d..e29675b 100644 --- a/buffet/string_utils.h +++ b/buffet/string_utils.h
@@ -36,6 +36,20 @@ std::string Join(const std::string& delimiter, const std::string& str1, const std::string& str2); +// string_utils::ToString() is a helper function to convert any scalar type +// to a string. In most cases, it redirects the call to std::to_string with +// two exceptions: for std::string itself and for double and bool. +template<typename T> +inline std::string ToString(T value) { return std::to_string(value); } +// Having the following overload is handy for templates where the type +// of template parameter isn't known and could be a string itself. +inline std::string ToString(std::string value) { return value; } +// We overload this for double because std::to_string(double) uses %f to +// format the value and I would like to use a shorter %g format instead. +std::string ToString(double value); +// And the bool to be converted as true/false instead of 1/0. +std::string ToString(bool value); + } // namespace string_utils } // namespace buffet