configure cppcheck (#622)

Also skip some search tests for Capella

.cppcheck_suppressions.txt

@@ -0,0 +1,36 @@
+# these look important
+duplInheritedMember:*
+shadowFunction:*
+templateRecursion:*
+useInitializationList:*
+
+# uncomment one-by-one and fix when have time
+constParameterCallback:*
+constParameterReference:*
+constVariableReference:*
+functionConst:*
+functionStatic:*
+noExplicitConstructor:*
+passedByValue:*
+returnByReference:*
+shadowVariable:*
+stlIfStrFind:*
+unusedFunction:*
+unusedStructMember:*
+unusedVariable:*
+useStlAlgorithm:*
+
+# these are not very useful
+checkersReport:*
+internalAstError:*
+missingInclude:*
+missingIncludeSystem:*
+unmatchedSuppression:*
+
+# suppressions in tests
+comparisonOfFuncReturningBoolError:*/test/test_*
+unknownMacro:*/test/test_*
+variableScope:*/test/test_*
+
+# do not scan third-party code
+*:*/_deps/*

.github/workflows/linters.yml

@@ -53,3 +53,16 @@ jobs:
         with:
           name: report
           path: cmake-build-report.tar.gz
+
+  cppcheck:
+    runs-on: ubuntu-24.04
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          submodules: recursive
+      - name: Install dependencies
+        run: |
+          sudo apt-get update -y
+          sudo apt-get install -y libssl-dev cmake curl wget gnupg2 clang clang-tools cppcheck
+      - name: Run cppcheck
+        run: ./bin/check-cppcheck

CMakeLists.txt

@@ -435,7 +435,7 @@ add_library(couchbase_cxx_client::couchbase_cxx_client ALIAS couchbase_cxx_clien
 set_target_properties(couchbase_cxx_client PROPERTIES POSITION_INDEPENDENT_CODE ON)
 
 target_include_directories(couchbase_cxx_client PRIVATE ${PROJECT_BINARY_DIR}/generated ${PROJECT_BINARY_DIR}/generated_$<CONFIG>)
-target_include_directories(couchbase_cxx_client PUBLIC ${PROJECT_SOURCE_DIR} ${PROJECT_SOURCE_DIR}/private)
+target_include_directories(couchbase_cxx_client PUBLIC ${PROJECT_SOURCE_DIR})
 target_include_directories(couchbase_cxx_client SYSTEM PUBLIC ${PROJECT_SOURCE_DIR}/third_party/cxx_function
                                                               ${PROJECT_SOURCE_DIR}/third_party/expected/include)
 target_link_libraries(couchbase_cxx_client PRIVATE project_options project_warnings)

bin/check-cppcheck

@@ -0,0 +1,42 @@
+#!/usr/bin/env bash
+
+#  Copyright 2020-2021 Couchbase, Inc.
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+PROJECT_ROOT="$( cd "$(dirname "$0"/..)" >/dev/null 2>&1 ; pwd -P )"
+
+CB_CMAKE=${CB_CMAKE:-$(which cmake)}
+CB_CC=${CB_CC:-$(which clang)}
+CB_CXX=${CB_CXX:-$(which clang++)}
+CB_NUMBER_OF_JOBS=${CB_NUMBER_OF_JOBS:-2}
+
+echo "CB_CC=${CB_CC}"
+echo "CB_CXX=${CB_CXX}"
+echo "CB_CMAKE=${CB_CMAKE}"
+
+BUILD_DIR="${PROJECT_ROOT}/cmake-build-cppcheck"
+
+set -exuo pipefail
+
+rm -rf "${BUILD_DIR}"
+mkdir -p "${BUILD_DIR}"
+cd "${BUILD_DIR}"
+
+${CB_CMAKE} -DENABLE_CPPCHECK=ON -DCMAKE_C_COMPILER=${CB_CC} -DCMAKE_CXX_COMPILER=${CB_CXX} ..
+set +e
+${CB_CMAKE} --build . --parallel ${CB_NUMBER_OF_JOBS} --verbose
+STATUS=$?
+set -e
+
+exit ${STATUS}

cmake/StaticAnalyzers.cmake

@@ -5,14 +5,20 @@ option(ENABLE_INCLUDE_WHAT_YOU_USE "Enable static analysis with include-what-you
 if(ENABLE_CPPCHECK)
   find_program(CPPCHECK cppcheck)
   if(CPPCHECK)
+    set(CPPCHECK_EXTRA_ARGS "")
+    if(ENABLE_CPPCHECK_INCONCLUSIVE)
+      set(CPPCHECK_EXTRA_ARGS "${CPPCHECK_EXTRA_ARGS} --inconclusive")
+    endif()
     set(CMAKE_CXX_CPPCHECK
         ${CPPCHECK}
-        --suppress=missingInclude
+        --error-exitcode=42
+        --verbose
+        --std=c++17
         --enable=all
         --inline-suppr
-        --inconclusive
-        -i
-        ${CMAKE_SOURCE_DIR}/imgui/lib)
+        --check-level=exhaustive
+        --suppressions-list=${PROJECT_SOURCE_DIR}/.cppcheck_suppressions.txt
+        ${CPPCHECK_EXTRA_ARGS})
   else()
     message(SEND_ERROR "cppcheck requested but executable not found")
   endif()

core/impl/observe_seqno.hxx

@@ -29,7 +29,7 @@ namespace couchbase::core::impl
 {
 struct observe_seqno_response {
   key_value_error_context ctx{};
-  bool active;
+  bool active{};
   std::uint16_t partition{};
   std::uint64_t partition_uuid{};
   std::uint64_t last_persisted_sequence_number{};
@@ -51,8 +51,8 @@ struct observe_seqno_request {
     core::protocol::client_response<core::protocol::observe_seqno_response_body>;
 
   core::document_id id;
-  bool active;
-  std::uint64_t partition_uuid;
+  bool active{};
+  std::uint64_t partition_uuid{};
   std::optional<std::chrono::milliseconds> timeout{};
   std::uint16_t partition{};
   std::uint32_t opaque{};

core/impl/query_index_manager.cxx

@@ -134,7 +134,7 @@ class watch_context : public std::enable_shared_from_this<watch_context>
         finish(resp, couchbase::errc::common::index_not_found);
         return true;
       }
-      complete &= (it != resp.indexes.end() && it->state == "online");
+      complete &= it->state == "online";
     }
     if (options_.watch_primary) {
       const auto it = std::find_if(resp.indexes.begin(),

core/impl/replica_utils.cxx

@@ -38,9 +38,9 @@ effective_nodes(const document_id& id,
   std::vector<readable_node> local_nodes{};
 
   for (std::size_t idx = 0U; idx <= config.num_replicas.value_or(0U); ++idx) {
-    bool is_replica = idx != 0;
     auto [vbid, server] = config.map_key(id.key(), idx);
     if (server.has_value() && server.value() < config.nodes.size()) {
+      bool is_replica = idx != 0;
       available_nodes.emplace_back(readable_node{ is_replica, idx });
       if (preferred_server_group == config.nodes[server.value()].server_group) {
         local_nodes.emplace_back(readable_node{ is_replica, idx });

core/impl/subdoc/lookup_in_specs.cxx

@@ -24,8 +24,8 @@ namespace couchbase
 auto
 lookup_in_specs::specs() const -> const std::vector<core::impl::subdoc::command>&
 {
-  static std::vector<core::impl::subdoc::command> empty{};
   if (specs_ == nullptr) {
+    static std::vector<core::impl::subdoc::command> empty{};
     return empty;
   }
   return specs_->specs();

core/impl/subdoc/mutate_in_specs.cxx

@@ -24,8 +24,8 @@ namespace couchbase
 auto
 mutate_in_specs::specs() const -> const std::vector<core::impl::subdoc::command>&
 {
-  static std::vector<core::impl::subdoc::command> empty{};
   if (specs_ == nullptr) {
+    static std::vector<core::impl::subdoc::command> empty{};
     return empty;
   }
   return specs_->specs();

core/impl/transaction_get_result.cxx

@@ -39,22 +39,28 @@ transaction_get_result::id() const -> const std::string&
   return base_->key();
 }
 
+// FIXME(SA): cppcheck complains about returning temporary strings in the
+// functions below, but it is not clear where does it see temporary here,
+// as all calls go down to the member fields of the base_.
+//
+// Curiously it does not complain about the base_->key()
+
 auto
 transaction_get_result::bucket() const -> const std::string&
 {
-  return base_->bucket();
+  return base_->bucket(); // cppcheck-suppress returnTempReference
 }
 
 auto
 transaction_get_result::scope() const -> const std::string&
 {
-  return base_->scope();
+  return base_->scope(); // cppcheck-suppress returnTempReference
 }
 
 auto
 transaction_get_result::collection() const -> const std::string&
 {
-  return base_->collection();
+  return base_->collection(); // cppcheck-suppress returnTempReference
 }
 
 auto

core/io/config_tracker.cxx

@@ -1,3 +1,20 @@
+/* -*- Mode: C++; tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+ *   Copyright 2020-Present Couchbase, Inc.
+ *
+ *   Licensed under the Apache License, Version 2.0 (the "License");
+ *   you may not use this file except in compliance with the License.
+ *   You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   Unless required by applicable law or agreed to in writing, software
+ *   distributed under the License is distributed on an "AS IS" BASIS,
+ *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *   See the License for the specific language governing permissions and
+ *   limitations under the License.
+ */
+
 #include "config_tracker.hxx"
 
 #include "core/impl/bootstrap_state_listener.hxx"
@@ -265,14 +282,19 @@ class cluster_config_tracker_impl
     {
       std::scoped_lock lock(sessions_mutex_);
 
+      if (sessions_.empty()) {
+        CB_LOG_WARNING(R"({} unable to find connected session (sessions_ is empty), retry in {})",
+                       log_prefix_,
+                       heartbeat_interval_);
+        return;
+      }
+
       std::size_t start = heartbeat_next_index_.fetch_add(1);
       std::size_t i = start;
       do {
-        if (!sessions_.empty()) {
-          std::size_t session_idx = i % sessions_.size();
-          if (sessions_[session_idx].is_bootstrapped() && sessions_[session_idx].supports_gcccp()) {
-            session = sessions_[session_idx];
-          }
+        std::size_t session_idx = i % sessions_.size();
+        if (sessions_[session_idx].is_bootstrapped() && sessions_[session_idx].supports_gcccp()) {
+          session = sessions_[session_idx];
         }
         i = heartbeat_next_index_.fetch_add(1);
       } while (start % sessions_.size() != i % sessions_.size());
@@ -654,4 +676,4 @@ cluster_config_tracker::supported_features() const -> std::vector<protocol::hell
   return impl_->supported_features();
 }
 
-} // namespace couchbase::core::io
+} // namespace couchbase::core::io

core/io/config_tracker.hxx

@@ -28,6 +28,7 @@
 #include <mutex>
 #include <optional>
 #include <string>
+#include <system_error>
 #include <vector>
 
 namespace asio
@@ -90,7 +91,7 @@ public:
                          asio::io_context& ctx,
                          asio::ssl::context& tls,
                          std::shared_ptr<impl::bootstrap_state_listener> state_listener);
-  ~cluster_config_tracker();
+  ~cluster_config_tracker() override;
 
   void create_sessions(
     utils::movable_function<void(std::error_code, topology::configuration cfg)>&& handler);

core/io/http_command.hxx

@@ -228,12 +228,12 @@ private:
         if (ec == asio::error::operation_aborted) {
           return self->invoke_handler(errc::common::ambiguous_timeout, std::move(msg));
         }
-        static std::string meter_name = "db.couchbase.operations";
-        static std::map<std::string, std::string> tags = {
-          { "db.couchbase.service", fmt::format("{}", self->request.type) },
-          { "db.operation", self->encoded.path },
-        };
         if (self->meter_) {
+          static std::string meter_name = "db.couchbase.operations";
+          static std::map<std::string, std::string> tags = {
+            { "db.couchbase.service", fmt::format("{}", self->request.type) },
+            { "db.operation", self->encoded.path },
+          };
           self->meter_->get_value_recorder(meter_name, tags)
             ->record_value(std::chrono::duration_cast<std::chrono::microseconds>(
                              std::chrono::steady_clock::now() - start)

core/io/http_session.cxx

@@ -294,12 +294,11 @@ http_session::cancel_current_response(std::error_code ec)
   std::scoped_lock lock(current_response_mutex_);
   if (streaming_response_) {
     auto ctx = std::move(current_streaming_response_);
-    if (ctx.resp_handler) {
-      ctx.resp_handler(ec, {});
+    if (auto handler = ctx.resp_handler; handler) {
+      handler(ec, {});
     }
-    if (ctx.stream_end_handler) {
-      ctx.stream_end_handler();
-      ctx.stream_end_handler = nullptr;
+    if (auto handler = std::move(ctx.stream_end_handler); handler) {
+      handler();
     }
   } else {
     if (auto ctx = std::move(current_response_); ctx.handler) {

core/io/mcbp_parser.cxx

@@ -65,8 +65,8 @@ mcbp_parser::next(mcbp_message& msg) -> mcbp_parser::result
                            body_size - prefix_size,
                            &uncompressed)) {
       msg.body.insert(msg.body.end(),
-                      reinterpret_cast<std::byte*>(&uncompressed.data()[0]),
-                      reinterpret_cast<std::byte*>(&uncompressed.data()[uncompressed.size()]));
+                      reinterpret_cast<std::byte*>(uncompressed.data()),
+                      reinterpret_cast<std::byte*>(uncompressed.data() + uncompressed.size()));
       use_raw_value = false;
       // patch header with new body size
       msg.header.bodylen =

core/logger/logger.cxx

@@ -189,14 +189,6 @@ create_file_logger_impl(const std::string logger_name, const configuration& logg
 {
   std::shared_ptr<spdlog::logger> logger{};
 
-  auto fname = logger_settings.filename;
-  auto buffersz = logger_settings.buffer_size;
-  auto cyclesz = logger_settings.cycle_size;
-
-  if (!spdlog::details::os::getenv("COUCHBASE_CXX_CLIENT_MAXIMIZE_LOGGER_CYCLE_SIZE").empty()) {
-    cyclesz = 1024LLU * 1024 * 1024; // use up to 1 GB log file size
-  }
-
   try {
     // Initialise the loggers.
     //
@@ -228,7 +220,13 @@ create_file_logger_impl(const std::string logger_name, const configuration& logg
     auto sink = std::make_shared<spdlog::sinks::dist_sink_mt>();
     sink->set_level(spdlog::level::trace);
 
-    if (!fname.empty()) {
+    if (const auto& fname = logger_settings.filename; !fname.empty()) {
+      auto cyclesz = logger_settings.cycle_size;
+
+      if (!spdlog::details::os::getenv("COUCHBASE_CXX_CLIENT_MAXIMIZE_LOGGER_CYCLE_SIZE").empty()) {
+        cyclesz = 1024LLU * 1024 * 1024; // use up to 1 GB log file size
+      }
+
       auto fsink = std::make_shared<custom_rotating_file_sink_mt>(fname, cyclesz, log_pattern);
       fsink->set_level(spdlog::level::trace);
       sink->add_sink(fsink);
@@ -256,6 +254,7 @@ create_file_logger_impl(const std::string logger_name, const configuration& logg
     if (logger_settings.unit_test) {
       logger = std::make_shared<spdlog::logger>(logger_name, sink);
     } else {
+      auto buffersz = logger_settings.buffer_size;
       // Create the default thread pool for async logging
       spdlog::init_thread_pool(buffersz, 1);
 

core/operations/document_lookup_in_any_replica.hxx

@@ -114,17 +114,6 @@ struct lookup_in_any_replica_request {
                 make_key_value_error_context(ec, id), ec, {}, {}, false) });
             }
 
-            if (ec) {
-              std::optional<std::string> first_error_path{};
-              std::optional<std::size_t> first_error_index{};
-              return h(
-                response_type{ make_subdocument_error_context(make_key_value_error_context(ec, id),
-                                                              ec,
-                                                              first_error_path,
-                                                              first_error_index,
-                                                              false) });
-            }
-
             using handler_type = utils::movable_function<void(response_type)>;
 
             struct replica_context {