clang/tidy: Fixes

Signed-off-by: Ryan Northey <ryan@synca.io>

.clang-tidy

@@ -56,9 +56,36 @@ 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$|
+    |^Ip6(ntohl|htonl)$|
+    |^get_$|
+    |^HeaderHasValue(Ref)?$|
+    |^HeaderValueOf$|
+    |^Is(Superset|Subset)OfHeaders$|
+    |^LLVMFuzzerInitialize$|
+    |^LLVMFuzzerTestOneInput$|
+    |^MOCK_METHOD$|
+    |^PrepareCall$|
+    |^PrintTo$|
+    |^resolve_dns$|
+    |^result_type$|
+    |Returns(Default)?WorkerId$|
+    |^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 +94,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/.*'
+
 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/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.

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,6 +78,7 @@ class QuicListenerFilterManagerImpl : public Network::QuicListenerFilterManager,
   }
   bool shouldAdvertiseServerPreferredAddress(
       const quic::QuicSocketAddress& server_preferred_address) const override {
+    // NOLINTNEXTLINE(modernize-loop-convert)
     for (auto iter = accept_filters_.begin(); iter != accept_filters_.end(); iter++) {
       if (!(*iter)->isCompatibleWithServerPreferredAddress(server_preferred_address)) {
         return false;
@@ -87,6 +88,7 @@ class QuicListenerFilterManagerImpl : public Network::QuicListenerFilterManager,
   }
   void onPeerAddressChanged(const quic::QuicSocketAddress& new_address,
                             Network::Connection& connection) override {
+    // NOLINTNEXTLINE(modernize-loop-convert)
     for (auto iter = accept_filters_.begin(); iter != accept_filters_.end(); iter++) {
       Network::FilterStatus status = (*iter)->onPeerAddressChanged(new_address, connection);
       if (status == Network::FilterStatus::StopIteration ||
@@ -96,6 +98,7 @@ class QuicListenerFilterManagerImpl : public Network::QuicListenerFilterManager,
     }
   }
   void startFilterChain() {
+    // NOLINTNEXTLINE(modernize-loop-convert)
     for (auto iter = accept_filters_.begin(); iter != accept_filters_.end(); iter++) {
       Network::FilterStatus status = (*iter)->onAccept(*this);
       if (status == Network::FilterStatus::StopIteration || !socket().ioHandle().isOpen()) {

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