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
