diff --git a/.clang-tidy b/.clang-tidy index 55d1c2389161..0794aa66661f 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -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 diff --git a/source/common/filesystem/filesystem_impl.cc b/source/common/filesystem/filesystem_impl.cc index d0e2dea40dae..90d44982026e 100644 --- a/source/common/filesystem/filesystem_impl.cc +++ b/source/common/filesystem/filesystem_impl.cc @@ -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( diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index 278eca02bba2..8e306f7b6e19 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -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) return; } // Technically this should be diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 47f7c137cdd8..281255c0c0e1 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -404,7 +404,7 @@ 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) { @@ -412,7 +412,7 @@ void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, const std::strin 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) { @@ -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) { @@ -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) { @@ -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 { diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index ce520bb3f95b..d0af5bf0829b 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -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 { diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 000409d92c6e..672662261689 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -218,9 +218,13 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( std::shared_ptr 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 = ¢ral_cache_map[stat->nameCStr()]; diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index fcfc9238bb1b..cb0e2363c496 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -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(); } diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index 028ec2afada4..03a8a18d26f7 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -93,7 +93,7 @@ const std::string Annotation::toJson() { if (endpoint_) { Util::mergeJsons(json_string, static_cast(endpoint_.value()).toJson(), - ZipkinJsonFieldNames::get().ANNOTATION_ENDPOINT.c_str()); + ZipkinJsonFieldNames::get().ANNOTATION_ENDPOINT); } return json_string; @@ -133,7 +133,7 @@ const std::string BinaryAnnotation::toJson() { if (endpoint_) { Util::mergeJsons(json_string, static_cast(endpoint_.value()).toJson(), - ZipkinJsonFieldNames::get().BINARY_ANNOTATION_ENDPOINT.c_str()); + ZipkinJsonFieldNames::get().BINARY_ANNOTATION_ENDPOINT); } return json_string; @@ -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 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; } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 5b69e165ed0d..19d6cec7f27f 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -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()); @@ -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); @@ -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); diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index 239e5f28738c..a534e6b6e9c0 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -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":")" +