clang/tidy: Fixes (the rest) (envoyproxy#29660)

Signed-off-by: Ryan Northey <ryan@synca.io>
Co-authored-by: wbpcode <wangbaiping@corp.netease.com>

.clang-tidy

@@ -56,9 +56,39 @@ CheckOptions:
   value: 'CamelCase'
 # Ignore GoogleTest function macros.
 - key: readability-identifier-naming.FunctionIgnoredRegexp
-  value: '(TEST|TEST_F|TEST_P|INSTANTIATE_TEST_SUITE_P|MOCK_METHOD|TYPED_TEST)'
+  # To have the regex chomped correctly fence all items with `|` (other than first/last)
+  value: >-
+    (^AbslHashValue$|
+    |^called_count$|
+    |^case_sensitive$|
+    |^Create$|
+    |^envoy_resolve_dns$|
+    |^evconnlistener_free$|
+    |^event_base_free$|
+    |^(get|set)EVP_PKEY$|
+    |^has_value$|
+    |^Ip6(ntohl|htonl)$|
+    |^get_$|
+    |^HeaderHasValue(Ref)?$|
+    |^HeaderValueOf$|
+    |^Is(Superset|Subset)OfHeaders$|
+    |^LLVMFuzzerInitialize$|
+    |^LLVMFuzzerTestOneInput$|
+    |^Locality$|
+    |^MOCK_METHOD$|
+    |^PrepareCall$|
+    |^PrintTo$|
+    |^resolve_dns$|
+    |^result_type$|
+    |Returns(Default)?WorkerId$|
+    |^sched_getaffinity$|
+    |^shutdownThread_$|
+    |TEST|
+    |^use_count$)
 - key: readability-identifier-naming.ParameterCase
   value: 'lower_case'
+- key: readability-identifier-naming.ParameterIgnoredRegexp
+  value: (^cname_ttl_$)
 - key: readability-identifier-naming.PrivateMemberCase
   value: 'lower_case'
 - key: readability-identifier-naming.PrivateMemberSuffix
@@ -67,11 +97,21 @@ CheckOptions:
   value: 'CamelCase'
 - key: readability-identifier-naming.TypeAliasCase
   value: 'CamelCase'
+- key: readability-identifier-naming.TypeAliasIgnoredRegexp
+  value: '(result_type)'
 - key: readability-identifier-naming.UnionCase
   value: 'CamelCase'
 - key: readability-identifier-naming.FunctionCase
   value: 'camelBack'
 
+HeaderFilterRegex: '^./source/.*|^./contrib/.*|^./test/.*|^./envoy/.*'
+
 UseColor: true
 
 WarningsAsErrors: '*'
+
+## The version here is arbitrary since any change to this file will
+## trigger a full run of clang-tidy against all files.
+## It can be useful as it seems some header changes may not trigger the
+## expected rerun.
+# v0

contrib/kafka/filters/network/test/mesh/command_handlers/fetch_record_converter_unit_test.cc

@@ -77,7 +77,7 @@ TEST(FetchRecordConverterImpl, shouldProcessRecords) {
   const auto ptr = reinterpret_cast<const uint32_t*>(data->data() + record_count_offset);
   const uint32_t record_count = be32toh(*ptr);
   ASSERT_EQ(record_count, 3);
-}
+} // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
 
 // Here we check whether our manual implementation really works.
 // https://github.com/apache/kafka/blob/3.3.2/clients/src/main/java/org/apache/kafka/common/utils/Crc32C.java

envoy/api/io_error.h

@@ -64,7 +64,7 @@ template <typename ReturnValue> struct IoCallResult {
       : return_value_(return_value), err_(std::move(err)) {}
 
   IoCallResult(IoCallResult<ReturnValue>&& result) noexcept
-      : return_value_(result.return_value_), err_(std::move(result.err_)) {}
+      : return_value_(std::move(result.return_value_)), err_(std::move(result.err_)) {}
 
   virtual ~IoCallResult() = default;
 

envoy/buffer/buffer.h

@@ -647,7 +647,7 @@ class Reservation final {
   // The following are for use only by implementations of Buffer. Because c++
   // doesn't allow inheritance of friendship, these are just trying to make
   // misuse easy to spot in a code review.
-  static Reservation bufferImplUseOnlyConstruct(Instance& buffer) { return Reservation(buffer); }
+  static Reservation bufferImplUseOnlyConstruct(Instance& buffer) { return {buffer}; }
   decltype(slices_)& bufferImplUseOnlySlices() { return slices_; }
   ReservationSlicesOwnerPtr& bufferImplUseOnlySlicesOwner() { return slices_owner_; }
   void bufferImplUseOnlySetLength(uint64_t length) { length_ = length; }
@@ -708,9 +708,7 @@ class ReservationSingleSlice final {
   // The following are for use only by implementations of Buffer. Because c++
   // doesn't allow inheritance of friendship, these are just trying to make
   // misuse easy to spot in a code review.
-  static ReservationSingleSlice bufferImplUseOnlyConstruct(Instance& buffer) {
-    return ReservationSingleSlice(buffer);
-  }
+  static ReservationSingleSlice bufferImplUseOnlyConstruct(Instance& buffer) { return {buffer}; }
   RawSlice& bufferImplUseOnlySlice() { return slice_; }
   ReservationSlicesOwnerPtr& bufferImplUseOnlySliceOwner() { return slice_owner_; }
 };

envoy/common/io/io_uring.h

@@ -260,7 +260,7 @@ using IoUringSocketPtr = std::unique_ptr<IoUringSocket>;
  */
 class IoUringWorker : public ThreadLocal::ThreadLocalObject {
 public:
-  virtual ~IoUringWorker() = default;
+  ~IoUringWorker() override = default;
 
   /**
    * Return the current thread's dispatcher.

envoy/common/optref.h

@@ -48,12 +48,12 @@ template <class T> struct OptRef {
   /**
    * Helper to convert a OptRef into a ref. Behavior if !has_value() is undefined.
    */
-  T& ref() const { return *ptr_; }
+  T& ref() const { return *ptr_; } // NOLINT(clang-analyzer-core.uninitialized.UndefReturn)
 
   /**
    * Helper to dereference an OptRef. Behavior if !has_value() is undefined.
    */
-  T& operator*() const { return *ptr_; }
+  T& operator*() const { return *ptr_; } // NOLINT(clang-analyzer-core.uninitialized.UndefReturn)
 
   /**
    * @return true if the object has a value.

envoy/router/router.h

@@ -599,7 +599,7 @@ class VirtualCluster {
 
   static VirtualClusterStats generateStats(Stats::Scope& scope,
                                            const VirtualClusterStatNames& stat_names) {
-    return VirtualClusterStats(stat_names, scope);
+    return {stat_names, scope};
   }
 };
 

envoy/stats/refcount_ptr.h

@@ -91,6 +91,7 @@ template <class T> class RefcountPtr {
 #endif
   bool operator==(const RefcountPtr& a) const { return ptr_ == a.ptr_; }
   bool operator!=(const RefcountPtr& a) const { return ptr_ != a.ptr_; }
+  // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDelete)
   uint32_t use_count() const { return ptr_->use_count(); }
   void reset() {
     resetInternal();
@@ -115,7 +116,7 @@ template <class T> class RefcountPtr {
 // forth.
 #if __cplusplus < 202002L
 template <class T> static bool operator==(std::nullptr_t, const RefcountPtr<T>& a) {
-  return a == nullptr;
+  return a == nullptr; // NOLINT(clang-analyzer-cplusplus.Move)
 }
 template <class T> static bool operator!=(std::nullptr_t, const RefcountPtr<T>& a) {
   return a != nullptr;

source/common/grpc/async_client_impl.cc

@@ -123,6 +123,7 @@ void AsyncStreamImpl::onHeaders(Http::ResponseHeaderMapPtr&& headers, bool end_s
       // TODO(mattklein123): clang-tidy is showing a use after move when passing to
       // onReceiveInitialMetadata() above. This looks like an actual bug that I will fix in a
       // follow up.
+      // NOLINTNEXTLINE(bugprone-use-after-move)
       onTrailers(Http::createHeaderMap<Http::ResponseTrailerMapImpl>(*headers));
       return;
     }

source/common/grpc/google_grpc_utils.cc

@@ -79,7 +79,7 @@ grpc::ByteBuffer GoogleGrpcUtils::makeByteBuffer(Buffer::InstancePtr&& buffer_in
     slices.emplace_back(raw_slice.mem_, raw_slice.len_,
                         &BufferInstanceContainer::derefBufferInstanceContainer, container);
   }
-  return {&slices[0], slices.size()};
+  return {&slices[0], slices.size()}; // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
 }
 
 class GrpcSliceBufferFragmentImpl : public Buffer::BufferFragment {

source/common/network/connection_balancer_impl.cc

@@ -28,7 +28,7 @@ ExactConnectionBalancerImpl::pickTargetHandler(BalancedConnectionHandler&) {
       }
     }
 
-    min_connection_handler->incNumConnections();
+    min_connection_handler->incNumConnections(); // NOLINT(clang-analyzer-core.CallAndMessage)
   }
 
   return *min_connection_handler;

source/common/quic/envoy_quic_server_connection.h

@@ -78,26 +78,26 @@ class QuicListenerFilterManagerImpl : public Network::QuicListenerFilterManager,
   }
   bool shouldAdvertiseServerPreferredAddress(
       const quic::QuicSocketAddress& server_preferred_address) const override {
-    for (auto iter = accept_filters_.begin(); iter != accept_filters_.end(); iter++) {
-      if (!(*iter)->isCompatibleWithServerPreferredAddress(server_preferred_address)) {
+    for (const auto& accept_filter : accept_filters_) {
+      if (!accept_filter->isCompatibleWithServerPreferredAddress(server_preferred_address)) {
         return false;
       }
     }
     return true;
   }
   void onPeerAddressChanged(const quic::QuicSocketAddress& new_address,
                             Network::Connection& connection) override {
-    for (auto iter = accept_filters_.begin(); iter != accept_filters_.end(); iter++) {
-      Network::FilterStatus status = (*iter)->onPeerAddressChanged(new_address, connection);
+    for (auto& accept_filter : accept_filters_) {
+      Network::FilterStatus status = accept_filter->onPeerAddressChanged(new_address, connection);
       if (status == Network::FilterStatus::StopIteration ||
           connection.state() != Network::Connection::State::Open) {
         return;
       }
     }
   }
   void startFilterChain() {
-    for (auto iter = accept_filters_.begin(); iter != accept_filters_.end(); iter++) {
-      Network::FilterStatus status = (*iter)->onAccept(*this);
+    for (auto& accept_filter : accept_filters_) {
+      Network::FilterStatus status = accept_filter->onAccept(*this);
       if (status == Network::FilterStatus::StopIteration || !socket().ioHandle().isOpen()) {
         break;
       }

source/common/rds/rds_route_config_subscription.cc

@@ -52,7 +52,7 @@ RdsRouteConfigSubscription::~RdsRouteConfigSubscription() {
 absl::Status RdsRouteConfigSubscription::onConfigUpdate(
     const std::vector<Envoy::Config::DecodedResourceRef>& resources,
     const std::string& version_info) {
-  if (resources.size() == 0) {
+  if (resources.empty()) {
     ENVOY_LOG(debug, "Missing {} RouteConfiguration for {} in onConfigUpdate()", rds_type_,
               route_config_name_);
     stats_.update_empty_.inc();

source/common/stats/symbol_table.h

@@ -702,7 +702,7 @@ class StatNameManagedStorage : public StatNameStorage {
   StatNameManagedStorage(StatName src, SymbolTable& table) noexcept
       : StatNameStorage(src, table), symbol_table_(table) {}
 
-  ~StatNameManagedStorage() { free(symbol_table_); }
+  ~StatNameManagedStorage() { free(symbol_table_); } // NOLINT(clang-analyzer-unix.Malloc)
 
 private:
   SymbolTable& symbol_table_;

source/common/stats/thread_local_store.cc

@@ -258,7 +258,7 @@ ThreadLocalStoreImpl::CentralCacheEntry::~CentralCacheEntry() {
   // is because many tests will not populate rejected_stats_.
   ASSERT(symbol_table_.toString(StatNameManagedStorage("Hello.world", symbol_table_).statName()) ==
          "Hello.world");
-  rejected_stats_.free(symbol_table_);
+  rejected_stats_.free(symbol_table_); // NOLINT(clang-analyzer-unix.Malloc)
 }
 
 void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) {

source/common/upstream/load_balancer_impl.cc

@@ -232,8 +232,10 @@ void LoadBalancerBase::recalculatePerPriorityState(uint32_t priority,
     // all hosts are healthy that priority's health is 100%*1.4=140% and is capped at 100% which
     // results in 100%. If 80% of hosts are healthy, that priority's health is still 100%
     // (80%*1.4=112% and capped at 100%).
-    per_priority_health.get()[priority] = std::min<uint32_t>(
-        100, (host_set.overprovisioningFactor() * healthy_weight / total_weight));
+    per_priority_health.get()[priority] =
+        std::min<uint32_t>(100,
+                           // NOLINTNEXTLINE(clang-analyzer-core.DivideZero)
+                           (host_set.overprovisioningFactor() * healthy_weight / total_weight));
 
     // We perform the same computation for degraded hosts.
     per_priority_degraded.get()[priority] = std::min<uint32_t>(

source/extensions/clusters/eds/eds.cc

@@ -163,7 +163,7 @@ void EdsClusterImpl::BatchUpdateHelper::updateLocalityEndpoints(
 absl::Status
 EdsClusterImpl::onConfigUpdate(const std::vector<Config::DecodedResourceRef>& resources,
                                const std::string&) {
-  if (resources.size() == 0) {
+  if (resources.empty()) {
     ENVOY_LOG(debug, "Missing ClusterLoadAssignment for {} in onConfigUpdate()", edsServiceName());
     info_->configUpdateStats().update_empty_.inc();
     onPreInitComplete();

source/server/hot_restarting_base.cc

@@ -157,6 +157,7 @@ bool RpcStream::replyIsExpectedType(const HotRestartMessage* proto,
 // should only get control data in a PassListenSocketReply, it should only be the fd passing type,
 // and there should only be one at a time. Crash on any other control data.
 void RpcStream::getPassedFdIfPresent(HotRestartMessage* out, msghdr* message) {
+  // NOLINTNEXTLINE(clang-analyzer-core.UndefinedBinaryOperatorResult)
   cmsghdr* cmsg = CMSG_FIRSTHDR(message);
   if (cmsg != nullptr) {
     RELEASE_ASSERT(cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS &&

source/server/options_impl.cc

@@ -54,6 +54,7 @@ OptionsImpl::OptionsImpl(std::vector<std::string> args,
                   "\nDefault is \"{}\"",
                   Logger::Logger::DEFAULT_LOG_FORMAT);
 
+  // NOLINTNEXTLINE(clang-analyzer-optin.cplusplus.VirtualCall)
   TCLAP::CmdLine cmd("envoy", ' ', VersionInfo::version());
   TCLAP::ValueArg<uint32_t> base_id(
       "", "base-id", "Base ID so that multiple envoys can run on the same host if needed", false, 0,

test/benchmark/main.cc

@@ -26,14 +26,14 @@ int main(int argc, char** argv) {
   bool contains_help_flag = false;
 
   // Checking if any of the command-line arguments contains `--help`
-  for (int i = 1; i < argc; ++i) {
+  for (int i = 1; i < argc; ++i) { // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall)
     if (strcmp(argv[i], "--help") == 0) {
       contains_help_flag = true;
       break;
     }
   }
 
-  if (contains_help_flag) {
+  if (contains_help_flag) { // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall)
     // if the `--help` flag isn't considered separately, it runs "benchmark --help"
     // (Google Benchmark Help) and the help output doesn't contains details about
     // custom defined flags like `--skip_expensive_benchmarks`, `--runtime_feature`, etc

test/common/access_log/access_log_manager_impl_test.cc

@@ -233,6 +233,7 @@ TEST_F(AccessLogManagerImplTest, FlushCountsIOErrors) {
 }
 
 TEST_F(AccessLogManagerImplTest, ReopenFile) {
+  // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
   NiceMock<Event::MockTimer>* timer = new NiceMock<Event::MockTimer>(&dispatcher_);
 
   Sequence sq;
@@ -275,12 +276,14 @@ TEST_F(AccessLogManagerImplTest, ReopenFile) {
   log_file->write("reopened");
   timer->invokeCallback();
 
+  // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
   EXPECT_TRUE(file_->waitForEventCount(file_->num_writes_, 2));
   EXPECT_TRUE(file_->waitForEventCount(file_->num_opens_, 2));
 }
 
 // Test that the `reopen()` will trigger file reopen even if no data is waiting.
 TEST_F(AccessLogManagerImplTest, ReopenFileNoWrite) {
+  // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
   NiceMock<Event::MockTimer>* timer = new NiceMock<Event::MockTimer>(&dispatcher_);
 
   Sequence sq;
@@ -299,6 +302,7 @@ TEST_F(AccessLogManagerImplTest, ReopenFileNoWrite) {
 
   log_file->write("before");
   timer->invokeCallback();
+  // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
   EXPECT_TRUE(file_->waitForEventCount(file_->num_writes_, 1));
 
   EXPECT_CALL(*file_, close_())
@@ -318,6 +322,7 @@ TEST_F(AccessLogManagerImplTest, ReopenFileNoWrite) {
 }
 
 TEST_F(AccessLogManagerImplTest, ReopenRetry) {
+  // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
   NiceMock<Event::MockTimer>* timer = new NiceMock<Event::MockTimer>(&dispatcher_);
 
   Sequence sq;
@@ -384,6 +389,7 @@ TEST_F(AccessLogManagerImplTest, ReopenRetry) {
   log_file->write("after reopen");
   timer->invokeCallback();
 
+  // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
   EXPECT_TRUE(file_->waitForEventCount(file_->num_writes_, 3));
   waitForCounterEq("filesystem.reopen_failed", 2);
   waitForGaugeEq("filesystem.write_total_buffered", 0);