lint: remove unnecessary calls to std::string::c_str() and std::strin…

…g::data() and fix use after move (#4971)

Signed-off-by: zyfjeff <[email protected]>

.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

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(

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

source/common/http/header_map_impl.cc

@@ -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) {
@@ -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 {

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 {

source/common/stats/thread_local_store.cc

@@ -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()];

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();
 }
 

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>(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>(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<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;
 }

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);

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":")" +