[WIP] ci: Re-enable clang-tidy

.azure-pipelines/pipelines.yml

@@ -65,6 +65,9 @@ stages:
 # Presubmit/default
 - ${{ if eq(variables.pipelineDefault, true) }}:
   - template: stages.yml
+    parameters:
+      checkStageDeps:
+      - env
 
 # Scheduled run anywhere
 - ${{ if eq(variables.pipelineScheduled, true) }}:

.azure-pipelines/stage/checks.yml

@@ -42,6 +42,12 @@ jobs:
     maxParallel: ${{ parameters.concurrencyChecks }}
     matrix:
       # These are ordered by most time-consuming first.
+      clang_tidy:
+        CI_TARGET: "clang_tidy"
+        REPO_FETCH_DEPTH: 0
+        REPO_FETCH_TAGS: true
+        PUBLISH_TEST_RESULTS: false
+        PUBLISH_ENVOY: false
       coverage:
         CI_TARGET: "coverage"
       fuzz_coverage:
@@ -62,16 +68,6 @@ jobs:
       msan:
         CI_TARGET: "msan"
         ENVOY_FILTER_EXAMPLE: true
-      #
-      # Temporarily disabled to facilitate release CI, should be resolved
-      #    as part of https://github.com/envoyproxy/envoy/issues/28566
-      #
-      # clang_tidy:
-      #   CI_TARGET: "clang_tidy"
-      #   REPO_FETCH_DEPTH: 0
-      #   REPO_FETCH_TAGS: true
-      #   PUBLISH_TEST_RESULTS: false
-      #   PUBLISH_ENVOY: false
       api:
         CI_TARGET: "api"
   timeoutInMinutes: 180

.bazelrc

@@ -106,6 +106,7 @@ build:clang-tidy --@envoy_toolshed//format/clang_tidy:config=//:clang_tidy_confi
 build:clang-tidy --aspects @envoy_toolshed//format/clang_tidy:clang_tidy.bzl%clang_tidy_aspect
 build:clang-tidy --output_groups=report
 build:clang-tidy --build_tag_filters=-notidy
+build:clang-tidy --cxxopt -fmacro-backtrace-limit=2
 
 # Basic ASAN/UBSAN that works for gcc
 build:asan --config=sanitizer

.clang-tidy

@@ -14,7 +14,6 @@ Checks: >
   clang-analyzer-core.DivideZero,
   misc-unused-using-decls,
   modernize-deprecated-headers,
-  modernize-loop-convert,
   modernize-make-shared,
   modernize-make-unique,
   modernize-return-braced-init-list,
@@ -39,6 +38,8 @@ Checks: >
   readability-redundant-smartptr-get,
   readability-redundant-string-cstr
 
+# modernize-loop-convert,
+
 CheckOptions:
 - key: cppcoreguidelines-unused-variable.IgnorePattern
   value: "^_$"

contrib/qat/compression/qatzstd/compressor/source/config.cc

@@ -23,8 +23,7 @@ QatzstdCompressorFactory::QatzstdCompressorFactory(
   }
 }
 
-QatzstdCompressorFactory::QatzstdThreadLocal::QatzstdThreadLocal()
-    : initialized_(false), sequenceProducerState_(nullptr) {}
+QatzstdCompressorFactory::QatzstdThreadLocal::QatzstdThreadLocal() = default;
 
 QatzstdCompressorFactory::QatzstdThreadLocal::~QatzstdThreadLocal() {
   if (initialized_) {
@@ -36,7 +35,7 @@ QatzstdCompressorFactory::QatzstdThreadLocal::~QatzstdThreadLocal() {
   }
 }
 
-void* QatzstdCompressorFactory::QatzstdThreadLocal::GetQATSession() {
+void* QatzstdCompressorFactory::QatzstdThreadLocal::getQATSession() {
   // The session must be initialized only once in every worker thread.
   if (!initialized_) {
 
@@ -57,7 +56,7 @@ void* QatzstdCompressorFactory::QatzstdThreadLocal::GetQATSession() {
 Envoy::Compression::Compressor::CompressorPtr QatzstdCompressorFactory::createCompressor() {
   return std::make_unique<QatzstdCompressorImpl>(
       compression_level_, enable_checksum_, strategy_, chunk_size_, enable_qat_zstd_,
-      qat_zstd_fallback_threshold_, enable_qat_zstd_ ? tls_slot_->get()->GetQATSession() : nullptr);
+      qat_zstd_fallback_threshold_, enable_qat_zstd_ ? tls_slot_->get()->getQATSession() : nullptr);
 }
 
 Envoy::Compression::Compressor::CompressorFactoryPtr

contrib/qat/compression/qatzstd/compressor/source/config.h

@@ -49,9 +49,9 @@ class QatzstdCompressorFactory : public Envoy::Compression::Compressor::Compress
                               public Logger::Loggable<Logger::Id::compression> {
     QatzstdThreadLocal();
     ~QatzstdThreadLocal() override;
-    void* GetQATSession();
-    bool initialized_;
-    void* sequenceProducerState_;
+    void* getQATSession();
+    bool initialized_{false};
+    void* sequenceProducerState_{nullptr};
   };
   const uint32_t compression_level_;
   const bool enable_checksum_;

contrib/qat/compression/qatzstd/compressor/source/qatzstd_compressor_impl.cc

@@ -10,12 +10,12 @@ QatzstdCompressorImpl::QatzstdCompressorImpl(uint32_t compression_level, bool en
                                              uint32_t strategy, uint32_t chunk_size,
                                              bool enable_qat_zstd,
                                              uint32_t qat_zstd_fallback_threshold,
-                                             void* sequenceProducerState)
+                                             void* sequence_producer_state)
     : ZstdCompressorImplBase(compression_level, enable_checksum, strategy, chunk_size),
       enable_qat_zstd_(enable_qat_zstd), qat_zstd_fallback_threshold_(qat_zstd_fallback_threshold),
-      sequenceProducerState_(sequenceProducerState), input_ptr_{std::make_unique<uint8_t[]>(
-                                                         chunk_size)},
-      input_len_(0), chunk_size_(chunk_size) {
+      sequence_producer_state_(sequence_producer_state), input_ptr_{std::make_unique<uint8_t[]>(
+                                                             chunk_size)},
+      chunk_size_(chunk_size) {
   size_t result;
   result = ZSTD_CCtx_setParameter(cctx_.get(), ZSTD_c_compressionLevel, compression_level_);
   RELEASE_ASSERT(!ZSTD_isError(result), "");
@@ -26,7 +26,7 @@ QatzstdCompressorImpl::QatzstdCompressorImpl(uint32_t compression_level, bool en
             compression_level, strategy, chunk_size, enable_qat_zstd, qat_zstd_fallback_threshold);
   if (enable_qat_zstd_) {
     /* register qatSequenceProducer */
-    ZSTD_registerSequenceProducer(cctx_.get(), sequenceProducerState_, qatSequenceProducer);
+    ZSTD_registerSequenceProducer(cctx_.get(), sequence_producer_state_, qatSequenceProducer);
     result = ZSTD_CCtx_setParameter(cctx_.get(), ZSTD_c_enableSeqProducerFallback, 1);
     RELEASE_ASSERT(!ZSTD_isError(result), "");
   }

contrib/qat/compression/qatzstd/compressor/source/qatzstd_compressor_impl.h

@@ -23,7 +23,7 @@ class QatzstdCompressorImpl : public Envoy::Compression::Zstd::Compressor::ZstdC
 public:
   QatzstdCompressorImpl(uint32_t compression_level, bool enable_checksum, uint32_t strategy,
                         uint32_t chunk_size, bool enable_qat_zstd,
-                        uint32_t qat_zstd_fallback_threshold, void* sequenceProducerState);
+                        uint32_t qat_zstd_fallback_threshold, void* sequence_producer_state);
 
 private:
   void compressPreprocess(Buffer::Instance& buffer,
@@ -38,9 +38,9 @@ class QatzstdCompressorImpl : public Envoy::Compression::Zstd::Compressor::ZstdC
 
   bool enable_qat_zstd_;
   const uint32_t qat_zstd_fallback_threshold_;
-  void* sequenceProducerState_;
+  void* sequence_producer_state_;
   std::unique_ptr<uint8_t[]> input_ptr_;
-  uint64_t input_len_;
+  uint64_t input_len_{0};
   uint64_t chunk_size_;
 };
 

envoy/http/filter.h

@@ -305,7 +305,7 @@ class RouteConfigUpdateRequester {
 class RouteConfigUpdateRequesterFactory : public Config::UntypedFactory {
 public:
   // UntypedFactory
-  virtual std::string category() const override { return "envoy.route_config_update_requester"; }
+  std::string category() const override { return "envoy.route_config_update_requester"; }
 
   virtual std::unique_ptr<RouteConfigUpdateRequester>
   createRouteConfigUpdateRequester(Router::RouteConfigProvider* route_config_provider) PURE;

source/common/common/posix/thread_impl.h

@@ -78,7 +78,8 @@ class PosixThreadFactory : public ThreadFactory {
    * Creates a new generic thread from the specified `thread_routine`. When the
    * thread cannot be created, this function will crash.
    */
-  ThreadPtr createThread(std::function<void()> thread_routine, OptionsOptConstRef options) PURE;
+  ThreadPtr createThread(std::function<void()> thread_routine,
+                         OptionsOptConstRef options) override PURE;
 
   /**
    * Creates a new POSIX thread from the specified `thread_routine`. When
@@ -93,7 +94,7 @@ class PosixThreadFactory : public ThreadFactory {
    * thread ID. The thread ID returned from this call is not the same as the
    * thread ID returned from `currentPThreadId()`.
    */
-  ThreadId currentThreadId() PURE;
+  ThreadId currentThreadId() override PURE;
 
   /** Returns the current pthread ID. It uses `pthread_self()`. */
   virtual ThreadId currentPthreadId() PURE;

source/common/http/conn_manager_impl.h

@@ -322,42 +322,39 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
 
     // All state for the stream. Put here for readability.
     struct State {
-      State()
-          : codec_saw_local_complete_(false), codec_encode_complete_(false),
-            on_reset_stream_called_(false), is_zombie_stream_(false), successful_upgrade_(false),
-            is_internally_destroyed_(false), is_internally_created_(false), is_tunneling_(false),
-            decorated_propagate_(true), deferred_to_next_io_iteration_(false) {}
+      State() = default;
 
       // It's possibly for the codec to see the completed response but not fully
       // encode it.
-      bool codec_saw_local_complete_ : 1; // This indicates that local is complete as the completed
-                                          // response has made its way to the codec.
-      bool codec_encode_complete_ : 1;    // This indicates that the codec has
-                                          // completed encoding the response.
-      bool on_reset_stream_called_ : 1;   // Whether the stream has been reset.
-      bool is_zombie_stream_ : 1;         // Whether stream is waiting for signal
-                                          // the underlying codec to be destroyed.
-      bool successful_upgrade_ : 1;
+      bool codec_saw_local_complete_ : 1 {
+          false}; // This indicates that local is complete as the completed
+                  // response has made its way to the codec.
+      bool codec_encode_complete_ : 1 {false};  // This indicates that the codec has
+                                                // completed encoding the response.
+      bool on_reset_stream_called_ : 1 {false}; // Whether the stream has been reset.
+      bool is_zombie_stream_ : 1 {false};       // Whether stream is waiting for signal
+                                                // the underlying codec to be destroyed.
+      bool successful_upgrade_ : 1 {false};
 
       // True if this stream was the original externally created stream, but was
       // destroyed as part of internal redirect.
-      bool is_internally_destroyed_ : 1;
+      bool is_internally_destroyed_ : 1 {false};
       // True if this stream is internally created. Currently only used for
       // internal redirects or other streams created via recreateStream().
-      bool is_internally_created_ : 1;
+      bool is_internally_created_ : 1 {false};
 
       // True if the response headers indicate a successful upgrade or connect
       // response.
-      bool is_tunneling_ : 1;
+      bool is_tunneling_ : 1 {false};
 
-      bool decorated_propagate_ : 1;
+      bool decorated_propagate_ : 1 {true};
 
       // Indicates that sending headers to the filter manager is deferred to the
       // next I/O cycle. If data or trailers are received when this flag is set
       // they are deferred too.
       // TODO(yanavlasov): encapsulate the entire state of deferred streams into a separate
       // structure, so it can be atomically created and cleared.
-      bool deferred_to_next_io_iteration_ : 1;
+      bool deferred_to_next_io_iteration_ : 1 {false};
       bool deferred_end_stream_ : 1;
     };
 

source/common/http/filter_manager.h

@@ -67,9 +67,8 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks,
   ActiveStreamFilterBase(FilterManager& parent, bool is_encoder_decoder_filter,
                          FilterContext filter_context)
       : parent_(parent), iteration_state_(IterationState::Continue),
-        filter_context_(std::move(filter_context)), iterate_from_current_filter_(false),
-        headers_continued_(false), continued_1xx_headers_(false), end_stream_(false),
-        is_encoder_decoder_filter_(is_encoder_decoder_filter), processed_headers_(false) {}
+        filter_context_(std::move(filter_context)),
+        is_encoder_decoder_filter_(is_encoder_decoder_filter) {}
 
   // Functions in the following block are called after the filter finishes processing
   // corresponding data. Those functions handle state updates and data storage (if needed)
@@ -185,14 +184,14 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks,
   // hasn't parsed data and trailers. As a result, the filter iteration should start with the
   // current filter instead of the next one. If true, filter iteration starts with the current
   // filter. Otherwise, starts with the next filter in the chain.
-  bool iterate_from_current_filter_ : 1;
-  bool headers_continued_ : 1;
-  bool continued_1xx_headers_ : 1;
+  bool iterate_from_current_filter_ : 1 {false};
+  bool headers_continued_ : 1 {false};
+  bool continued_1xx_headers_ : 1 {false};
   // If true, end_stream is called for this filter.
-  bool end_stream_ : 1;
+  bool end_stream_ : 1 {false};
   const bool is_encoder_decoder_filter_ : 1;
   // If true, the filter has processed headers.
-  bool processed_headers_ : 1;
+  bool processed_headers_ : 1 {false};
 };
 
 /**
@@ -864,35 +863,30 @@ class FilterManager : public ScopeTrackedObject,
 
 protected:
   struct State {
-    State()
-        : remote_decode_complete_(false), remote_encode_complete_(false), local_complete_(false),
-          has_1xx_headers_(false), created_filter_chain_(false), is_head_request_(false),
-          is_grpc_request_(false), non_100_response_headers_encoded_(false),
-          under_on_local_reply_(false), decoder_filter_chain_aborted_(false),
-          encoder_filter_chain_aborted_(false), saw_downstream_reset_(false) {}
+    State() = default;
     uint32_t filter_call_state_{0};
 
-    bool remote_decode_complete_ : 1;
-    bool remote_encode_complete_ : 1;
-    bool local_complete_ : 1; // This indicates that local is complete prior to filter processing.
-                              // A filter can still stop the stream from being complete as seen
-                              // by the codec.
+    bool remote_decode_complete_ : 1 {false};
+    bool remote_encode_complete_ : 1 {false};
+    bool local_complete_ : 1 {false}; // This indicates that local is complete prior to filter
+                                      // processing. A filter can still stop the stream from being
+                                      // complete as seen by the codec.
     // By default, we will assume there are no 1xx. If encode1xxHeaders
     // is ever called, this is set to true so commonContinue resumes processing the 1xx.
-    bool has_1xx_headers_ : 1;
-    bool created_filter_chain_ : 1;
+    bool has_1xx_headers_ : 1 {false};
+    bool created_filter_chain_ : 1 {false};
     // These two are latched on initial header read, to determine if the original headers
     // constituted a HEAD or gRPC request, respectively.
-    bool is_head_request_ : 1;
-    bool is_grpc_request_ : 1;
+    bool is_head_request_ : 1 {false};
+    bool is_grpc_request_ : 1 {false};
     // Tracks if headers other than 100-Continue have been encoded to the codec.
-    bool non_100_response_headers_encoded_ : 1;
+    bool non_100_response_headers_encoded_ : 1 {false};
     // True under the stack of onLocalReply, false otherwise.
-    bool under_on_local_reply_ : 1;
+    bool under_on_local_reply_ : 1 {false};
     // True when the filter chain iteration was aborted with local reply.
-    bool decoder_filter_chain_aborted_ : 1;
-    bool encoder_filter_chain_aborted_ : 1;
-    bool saw_downstream_reset_ : 1;
+    bool decoder_filter_chain_aborted_ : 1 {false};
+    bool encoder_filter_chain_aborted_ : 1 {false};
+    bool saw_downstream_reset_ : 1 {false};
 
     // The following 3 members are booleans rather than part of the space-saving bitfield as they
     // are passed as arguments to functions expecting bools. Extend State using the bitfield

source/common/http/http1/codec_impl.cc

@@ -100,9 +100,7 @@ static constexpr absl::string_view COLON_SPACE = ": ";
 
 StreamEncoderImpl::StreamEncoderImpl(ConnectionImpl& connection,
                                      StreamInfo::BytesMeterSharedPtr&& bytes_meter)
-    : connection_(connection), disable_chunk_encoding_(false), chunk_encoding_(true),
-      connect_request_(false), is_tcp_tunneling_(false), is_response_to_head_request_(false),
-      is_response_to_connect_request_(false), bytes_meter_(std::move(bytes_meter)) {
+    : connection_(connection), bytes_meter_(std::move(bytes_meter)) {
   if (!bytes_meter_) {
     bytes_meter_ = std::make_shared<StreamInfo::BytesMeter>();
   }
@@ -528,9 +526,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat
                                uint32_t max_headers_kb, const uint32_t max_headers_count)
     : connection_(connection), stats_(stats), codec_settings_(settings),
       encode_only_header_key_formatter_(encodeOnlyFormatterFromSettings(settings)),
-      processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false),
-      deferred_end_stream_headers_(false), dispatching_(false), max_headers_kb_(max_headers_kb),
-      max_headers_count_(max_headers_count) {
+      max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) {
   if (codec_settings_.use_balsa_parser_) {
     parser_ = std::make_unique<BalsaParser>(type, this, max_headers_kb_ * 1024, enableTrailers(),
                                             codec_settings_.allow_custom_methods_);

source/common/http/http1/codec_impl.h

@@ -99,12 +99,12 @@ class StreamEncoderImpl : public virtual StreamEncoder,
   Buffer::BufferMemoryAccountSharedPtr buffer_memory_account_;
   ConnectionImpl& connection_;
   uint32_t read_disable_calls_{};
-  bool disable_chunk_encoding_ : 1;
-  bool chunk_encoding_ : 1;
-  bool connect_request_ : 1;
-  bool is_tcp_tunneling_ : 1;
-  bool is_response_to_head_request_ : 1;
-  bool is_response_to_connect_request_ : 1;
+  bool disable_chunk_encoding_ : 1 {false};
+  bool chunk_encoding_ : 1 {true};
+  bool connect_request_ : 1 {false};
+  bool is_tcp_tunneling_ : 1 {false};
+  bool is_response_to_head_request_ : 1 {false};
+  bool is_response_to_connect_request_ : 1 {false};
 
 private:
   /**
@@ -308,14 +308,14 @@ class ConnectionImpl : public virtual Connection,
   const HeaderKeyFormatterConstPtr encode_only_header_key_formatter_;
   HeaderString current_header_field_;
   HeaderString current_header_value_;
-  bool processing_trailers_ : 1;
-  bool handling_upgrade_ : 1;
-  bool reset_stream_called_ : 1;
+  bool processing_trailers_ : 1 {false};
+  bool handling_upgrade_ : 1 {false};
+  bool reset_stream_called_ : 1 {false};
   // Deferred end stream headers indicate that we are not going to raise headers until the full
   // HTTP/1 message has been flushed from the parser. This allows raising an HTTP/2 style headers
   // block with end stream set to true with no further protocol data remaining.
-  bool deferred_end_stream_headers_ : 1;
-  bool dispatching_ : 1;
+  bool deferred_end_stream_headers_ : 1 {false};
+  bool dispatching_ : 1 {false};
   bool dispatching_slice_already_drained_ : 1;
   StreamInfo::BytesMeterSharedPtr bytes_meter_before_stream_;
   const uint32_t max_headers_kb_;