Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint: remove unnecessary calls to std::string::c_str() and std::string::data() and fix use after move #4971

Merged
merged 10 commits into from
Nov 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Checks: 'clang-diagnostic-*,clang-analyzer-*,abseil-*,bugprone-*,modernize-*,performance-*,readability-redundant-*,readability-braces-around-statements'

#TODO(lizan): grow this list, fix possible warnings and make more checks as error
WarningsAsErrors: 'bugprone-assert-side-effect,modernize-make-shared,modernize-make-unique,readability-redundant-smartptr-get,readability-braces-around-statements'
WarningsAsErrors: 'bugprone-assert-side-effect,modernize-make-shared,modernize-make-unique,readability-redundant-smartptr-get,readability-braces-around-statements,readability-redundant-string-cstr,bugprone-use-after-move'

CheckOptions:
- key: bugprone-assert-side-effect.AssertMacros
Expand Down
4 changes: 2 additions & 2 deletions source/common/filesystem/filesystem_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ FileImpl::FileImpl(const std::string& path, Event::Dispatcher& dispatcher,
}

void FileImpl::open() {
Api::SysCallIntResult result = os_sys_calls_.open(path_.c_str(), O_RDWR | O_APPEND | O_CREAT,
S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
Api::SysCallIntResult result =
os_sys_calls_.open(path_, O_RDWR | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
fd_ = result.rc_;
if (-1 == fd_) {
throw EnvoyException(
Expand Down
4 changes: 3 additions & 1 deletion source/common/grpc/async_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ void AsyncStreamImpl::onHeaders(Http::HeaderMapPtr&& headers, bool end_stream) {
// https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md requires that
// grpc-status be used if available.
if (end_stream && grpc_status) {
onTrailers(std::move(headers));
// There is actually no use-after-move problem here,
// because it will only be executed when end_stream is equal to true.
onTrailers(std::move(headers)); // NOLINT(bugprone-use-after-move)
zyfjeff marked this conversation as resolved.
Show resolved Hide resolved
return;
}
// Technically this should be
Expand Down
14 changes: 7 additions & 7 deletions source/common/http/header_map_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -404,15 +404,15 @@ void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, uint64_t value)
HeaderString new_value;
new_value.setInteger(value);
insertByKey(std::move(ref_key), std::move(new_value));
ASSERT(new_value.empty());
ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
}

void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, const std::string& value) {
HeaderString ref_key(key);
HeaderString new_value;
new_value.setCopy(value.c_str(), value.size());
insertByKey(std::move(ref_key), std::move(new_value));
ASSERT(new_value.empty());
ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
}

void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) {
Expand All @@ -428,8 +428,8 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) {
HeaderString new_value;
new_value.setInteger(value);
insertByKey(std::move(new_key), std::move(new_value));
ASSERT(new_key.empty());
ASSERT(new_value.empty());
ASSERT(new_key.empty()); // NOLINT(bugprone-use-after-move)
ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
}

void HeaderMapImpl::addCopy(const LowerCaseString& key, const std::string& value) {
Expand All @@ -443,8 +443,8 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, const std::string& value
HeaderString new_value;
new_value.setCopy(value.c_str(), value.size());
insertByKey(std::move(new_key), std::move(new_value));
ASSERT(new_key.empty());
ASSERT(new_value.empty());
ASSERT(new_key.empty()); // NOLINT(bugprone-use-after-move)
ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
}

void HeaderMapImpl::setReference(const LowerCaseString& key, const std::string& value) {
Expand All @@ -460,7 +460,7 @@ void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, const std::strin
new_value.setCopy(value.c_str(), value.size());
remove(key);
insertByKey(std::move(ref_key), std::move(new_value));
ASSERT(new_value.empty());
ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
}

uint64_t HeaderMapImpl::byteSize() const {
Expand Down
3 changes: 1 addition & 2 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -743,8 +743,7 @@ RegexRouteEntryImpl::RegexRouteEntryImpl(const VirtualHostImpl& vhost,
const envoy::api::v2::route::Route& route,
Server::Configuration::FactoryContext& factory_context)
: RouteEntryImplBase(vhost, route, factory_context),
regex_(RegexUtil::parseRegex(route.match().regex().c_str())),
regex_str_(route.match().regex()) {}
regex_(RegexUtil::parseRegex(route.match().regex())), regex_str_(route.match().regex()) {}

void RegexRouteEntryImpl::rewritePathHeader(Http::HeaderMap& headers,
bool insert_envoy_original_path) const {
Expand Down
8 changes: 6 additions & 2 deletions source/common/stats/thread_local_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,13 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
std::shared_ptr<StatType> stat =
make_stat(parent_.alloc_, truncated_name, std::move(tag_extracted_name), std::move(tags));
if (stat == nullptr) {
// TODO(jmarantz): If make_stat fails, the actual move does not actually occur
// for tag_extracted_name and tags, so there is no use-after-move problem.
// In order to increase the readability of the code, refactoring is done here.
parent_.num_last_resort_stats_.inc();
stat = make_stat(parent_.heap_allocator_, truncated_name, std::move(tag_extracted_name),
std::move(tags));
stat = make_stat(parent_.heap_allocator_, truncated_name,
std::move(tag_extracted_name), // NOLINT(bugprone-use-after-move)
std::move(tags)); // NOLINT(bugprone-use-after-move)
ASSERT(stat != nullptr);
}
central_ref = &central_cache_map[stat->nameCStr()];
Expand Down
5 changes: 3 additions & 2 deletions source/common/upstream/health_discovery_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,12 @@ void HdsDelegate::onReceiveMessage(
// Reset
hds_clusters_.clear();

// Set response
server_response_ms_ = PROTOBUF_GET_MS_REQUIRED(*message, interval);

// Process the HealthCheckSpecifier message
processMessage(std::move(message));

// Set response
server_response_ms_ = PROTOBUF_GET_MS_REQUIRED(*message, interval);
setHdsStreamResponseTimer();
}

Expand Down
8 changes: 4 additions & 4 deletions source/extensions/tracers/zipkin/zipkin_core_types.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ const std::string Annotation::toJson() {

if (endpoint_) {
Util::mergeJsons(json_string, static_cast<Endpoint>(endpoint_.value()).toJson(),
ZipkinJsonFieldNames::get().ANNOTATION_ENDPOINT.c_str());
ZipkinJsonFieldNames::get().ANNOTATION_ENDPOINT);
}

return json_string;
Expand Down Expand Up @@ -133,7 +133,7 @@ const std::string BinaryAnnotation::toJson() {

if (endpoint_) {
Util::mergeJsons(json_string, static_cast<Endpoint>(endpoint_.value()).toJson(),
ZipkinJsonFieldNames::get().BINARY_ANNOTATION_ENDPOINT.c_str());
ZipkinJsonFieldNames::get().BINARY_ANNOTATION_ENDPOINT);
}

return json_string;
Expand Down Expand Up @@ -207,14 +207,14 @@ const std::string Span::toJson() {
annotation_json_vector.push_back(it->toJson());
}
Util::addArrayToJson(json_string, annotation_json_vector,
ZipkinJsonFieldNames::get().SPAN_ANNOTATIONS.c_str());
ZipkinJsonFieldNames::get().SPAN_ANNOTATIONS);

std::vector<std::string> binary_annotation_json_vector;
for (auto it = binary_annotations_.begin(); it != binary_annotations_.end(); it++) {
binary_annotation_json_vector.push_back(it->toJson());
}
Util::addArrayToJson(json_string, binary_annotation_json_vector,
ZipkinJsonFieldNames::get().SPAN_BINARY_ANNOTATIONS.c_str());
ZipkinJsonFieldNames::get().SPAN_BINARY_ANNOTATIONS);

return json_string;
}
Expand Down
6 changes: 3 additions & 3 deletions test/common/http/header_map_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ TEST(HeaderStringTest, All) {
HeaderString string1(static_string);
HeaderString string2(std::move(string1));
EXPECT_STREQ("HELLO", string2.c_str());
EXPECT_EQ(static_string.c_str(), string1.c_str());
EXPECT_EQ(static_string.c_str(), string1.c_str()); // NOLINT(bugprone-use-after-move)
EXPECT_EQ(static_string.c_str(), string2.c_str());
EXPECT_EQ(5U, string1.size());
EXPECT_EQ(5U, string2.size());
Expand All @@ -50,7 +50,7 @@ TEST(HeaderStringTest, All) {
string.setCopy("hello", 5);
EXPECT_EQ(HeaderString::Type::Inline, string.type());
HeaderString string2(std::move(string));
EXPECT_TRUE(string.empty());
EXPECT_TRUE(string.empty()); // NOLINT(bugprone-use-after-move)
EXPECT_EQ(HeaderString::Type::Inline, string.type());
EXPECT_EQ(HeaderString::Type::Inline, string2.type());
string.append("world", 5);
Expand All @@ -67,7 +67,7 @@ TEST(HeaderStringTest, All) {
string.setCopy(large.c_str(), large.size());
EXPECT_EQ(HeaderString::Type::Dynamic, string.type());
HeaderString string2(std::move(string));
EXPECT_TRUE(string.empty());
EXPECT_TRUE(string.empty()); // NOLINT(bugprone-use-after-move)
EXPECT_EQ(HeaderString::Type::Inline, string.type());
EXPECT_EQ(HeaderString::Type::Dynamic, string2.type());
string.append("b", 1);
Expand Down
2 changes: 1 addition & 1 deletion test/extensions/tracers/zipkin/zipkin_core_types_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) {

// Test setSourceServiceName and setDestinationServiceName

ann.setValue(Zipkin::ZipkinCoreConstants::get().CLIENT_RECV);
ann.setValue(Zipkin::ZipkinCoreConstants::get().CLIENT_RECV); // NOLINT(bugprone-use-after-move)
span.addAnnotation(ann);
span.setServiceName("NEW_SERVICE_NAME");
EXPECT_EQ(R"({"traceId":")" + span.traceIdAsHexString() + R"(","name":"span_name","id":")" +
Expand Down