diff --git a/.clang-format b/.clang-format index 250b8e125..b9c31482d 100644 --- a/.clang-format +++ b/.clang-format @@ -29,4 +29,5 @@ KeepEmptyLinesAtTheStartOfBlocks: false PointerAlignment: Left ReferenceAlignment: Left ReflowComments: true +NamespaceMacros: ['SVS_DECLARE_NAMESPACE_VERSION'] --- diff --git a/.clang-tidy b/.clang-tidy index 2cbeea94a..5b53f3ac1 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,12 +1,60 @@ -Checks: "-*, - bugprone*, - -bugprone-easily-swappable-parameters, - -bugprone-macro-parentheses, - clang-analyzer*, - readability*, - -readability-magic-numbers, - -readability-identifier-length" +Checks: ' +-*, +bugprone-argument-comment, +bugprone-sizeof-*, +bugprone-use-after-move, +clang-diagnostic-*, +-clang-diagnostic-nrvo, +-clang-diagnostic-missing-designated-field-initializers, +cppcoreguidelines-pro-type-member-init, +cppcoreguidelines-special-member-functions, +google-build-using-namespace, +misc-definitions-in-headers, +modernize-use-emplace, +modernize-use-using, +performance-faster-string-find, +performance-for-range-copy, +performance-implicit-conversion-in-loop, +performance-inefficient-algorithm, +performance-inefficient-string-concatenation, +performance-inefficient-vector-operation, +performance-move-const-arg, +performance-move-constructor-init, +performance-no-automatic-move, +performance-no-int-to-ptr, +performance-noexcept-move-constructor, +performance-trivially-destructible, +performance-type-promotion-in-math-fn, +performance-unnecessary-copy-initialization, +performance-unnecessary-value-param, +readability-operators-representation, +readability-redundant-string-init, +readability-braces-around-statements, +' + +WarningsAsErrors: ' +bugprone-use-after-move, +' CheckOptions: - - key: readability-implicit-bool-conversion.AllowIntegerConditions - value: '1' +- key: bugprone-easily-swappable-parameters.MinimumLength + value: 4 +- key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor + value: true +- key: cppcoreguidelines-special-member-functions.AllowImplicitlyDeletedCopyOrMove + value: 1 +- key: modernize-use-using.IgnoreExternC + value: true +- key: performance-move-const-arg.CheckTriviallyCopyableMove + value: false +- key: performance-unnecessary-value-param.AllowedTypes + value: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$' +- key: performance-unnecessary-copy-initialization.AllowedTypes + value: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$' +- key: readability-operators-representation.BinaryOperators + value: '&&;&=;&;|;~;!;!=;||;|=;^;^=' +- key: readability-redundant-string-init.StringNames + value: '::std::basic_string' +- key: readability-named-parameter.InsertPlainNamesInForwardDecls + value: true +... diff --git a/.github/scripts/build-cpp-runtime-bindings.sh b/.github/scripts/build-cpp-runtime-bindings.sh index c71ff43b6..74ab7990f 100644 --- a/.github/scripts/build-cpp-runtime-bindings.sh +++ b/.github/scripts/build-cpp-runtime-bindings.sh @@ -41,6 +41,7 @@ CMAKE_ARGS=( "-DCMAKE_INSTALL_LIBDIR=lib" "-DSVS_RUNTIME_ENABLE_LVQ_LEANVEC=${ENABLE_LVQ_LEANVEC:-ON}" "-DSVS_RUNTIME_ENABLE_IVF=ON" + "-DSVS_EXPERIMENTAL_CLANG_TIDY=ON" ) if [ -n "$SVS_URL" ]; then diff --git a/bindings/cpp/CMakeLists.txt b/bindings/cpp/CMakeLists.txt index 85db0c724..db6053f97 100644 --- a/bindings/cpp/CMakeLists.txt +++ b/bindings/cpp/CMakeLists.txt @@ -82,6 +82,23 @@ target_compile_options(${TARGET_NAME} PRIVATE -fvisibility=hidden ) +# Optional clang-tidy integration. Reuses the top-level repo's +# cmake/clang-tidy.cmake module so the runtime build runs the same +# .clang-tidy config as the rest of the library. +option(SVS_EXPERIMENTAL_CLANG_TIDY + "Run the clang-tidy static analyzer on the cpp runtime bindings." + OFF +) +if(SVS_EXPERIMENTAL_CLANG_TIDY) + list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../../cmake") + include(clang-tidy) + if(CLANG_TIDY_COMMAND) + set_target_properties(${TARGET_NAME} + PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}" + ) + endif() +endif() + if(UNIX AND NOT APPLE) # Don't export 3rd-party symbols from the lib target_link_options(${TARGET_NAME} PRIVATE "SHELL:-Wl,--exclude-libs,ALL") @@ -142,6 +159,19 @@ if (SVS_RUNTIME_ENABLE_LVQ_LEANVEC) FetchContent_MakeAvailable(svs) list(APPEND CMAKE_PREFIX_PATH "${svs_SOURCE_DIR}") find_package(svs REQUIRED) + + # Strip svs::svs_compile_options gcc-only flags for clang-tidy + if(SVS_EXPERIMENTAL_CLANG_TIDY AND TARGET svs::svs_compile_options) + get_target_property(_svs_co_opts svs::svs_compile_options INTERFACE_COMPILE_OPTIONS) + if(_svs_co_opts) + list(FILTER _svs_co_opts EXCLUDE REGEX "^-fconcepts-diagnostics-depth") + list(FILTER _svs_co_opts EXCLUDE REGEX "^-ftemplate-backtrace-limit") + set_property(TARGET svs::svs_compile_options + PROPERTY INTERFACE_COMPILE_OPTIONS "${_svs_co_opts}" + ) + endif() + endif() + target_link_libraries(${TARGET_NAME} PRIVATE svs::svs svs::svs_compile_options diff --git a/bindings/cpp/include/svs/runtime/api_defs.h b/bindings/cpp/include/svs/runtime/api_defs.h index f77df9d78..504df76c4 100644 --- a/bindings/cpp/include/svs/runtime/api_defs.h +++ b/bindings/cpp/include/svs/runtime/api_defs.h @@ -32,8 +32,7 @@ namespace svs { namespace runtime { -namespace v0 { - +SVS_DECLARE_NAMESPACE_VERSION(0) { class OptionalBool { enum class Value : int8_t { Undef = -1, True = 1, False = 0 }; Value value_; @@ -206,7 +205,6 @@ struct SVS_RUNTIME_API_INTERFACE ResultsAllocator { return this->allocate(result_counts); } }; - -} // namespace v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) } // namespace runtime } // namespace svs diff --git a/bindings/cpp/include/svs/runtime/dynamic_ivf_index.h b/bindings/cpp/include/svs/runtime/dynamic_ivf_index.h index 39eda2af8..ebc35f75f 100644 --- a/bindings/cpp/include/svs/runtime/dynamic_ivf_index.h +++ b/bindings/cpp/include/svs/runtime/dynamic_ivf_index.h @@ -24,8 +24,7 @@ namespace svs { namespace runtime { -namespace v0 { - +SVS_DECLARE_NAMESPACE_VERSION(0) { /// @brief Abstract interface for dynamic IVF indices (supports add/delete). struct SVS_RUNTIME_API DynamicIVFIndex : public IVFIndex { /// @brief Utility function to check storage kind support. @@ -160,7 +159,6 @@ struct SVS_RUNTIME_API DynamicIVFIndexLeanVec : public DynamicIVFIndex { size_t intra_query_threads = 1 ) noexcept; }; - -} // namespace v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) } // namespace runtime } // namespace svs diff --git a/bindings/cpp/include/svs/runtime/dynamic_vamana_index.h b/bindings/cpp/include/svs/runtime/dynamic_vamana_index.h index 1d487baf9..fb588f6b6 100644 --- a/bindings/cpp/include/svs/runtime/dynamic_vamana_index.h +++ b/bindings/cpp/include/svs/runtime/dynamic_vamana_index.h @@ -25,8 +25,7 @@ namespace svs { namespace runtime { -namespace v0 { - +SVS_DECLARE_NAMESPACE_VERSION(0) { // Abstract interface for Dynamic Vamana-based indexes. struct SVS_RUNTIME_API DynamicVamanaIndex : public VamanaIndex { virtual Status add(size_t n, const size_t* labels, const float* x) noexcept = 0; @@ -34,8 +33,6 @@ struct SVS_RUNTIME_API DynamicVamanaIndex : public VamanaIndex { remove_selected(size_t* num_removed, const IDFilter& selector) noexcept = 0; virtual Status remove(size_t n, const size_t* labels) noexcept = 0; - virtual Status reset() noexcept = 0; - // Utility function to check storage kind support static Status check_storage_kind(StorageKind storage_kind) noexcept; @@ -65,7 +62,6 @@ struct SVS_RUNTIME_API DynamicVamanaIndex : public VamanaIndex { static Status destroy(DynamicVamanaIndex* index) noexcept; - virtual Status save(std::ostream& out) const noexcept = 0; static Status load( DynamicVamanaIndex** index, std::istream& in, @@ -131,7 +127,6 @@ struct SVS_RUNTIME_API DynamicVamanaIndexLeanVec : public DynamicVamanaIndex { const VamanaIndex::DynamicIndexParams& dynamic_index_params ) noexcept; }; - -} // namespace v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) } // namespace runtime } // namespace svs diff --git a/bindings/cpp/include/svs/runtime/flat_index.h b/bindings/cpp/include/svs/runtime/flat_index.h index 9c635e35d..c6bb947d2 100644 --- a/bindings/cpp/include/svs/runtime/flat_index.h +++ b/bindings/cpp/include/svs/runtime/flat_index.h @@ -23,8 +23,7 @@ namespace svs { namespace runtime { -namespace v0 { - +SVS_DECLARE_NAMESPACE_VERSION(0) { // Abstract interface for Flat indices. struct SVS_RUNTIME_API FlatIndex { // Utility function to check storage kind support @@ -45,7 +44,6 @@ struct SVS_RUNTIME_API FlatIndex { virtual Status save(std::ostream& out) const noexcept = 0; static Status load(FlatIndex** index, std::istream& in, MetricType metric) noexcept; }; - -} // namespace v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) } // namespace runtime } // namespace svs diff --git a/bindings/cpp/include/svs/runtime/ivf_index.h b/bindings/cpp/include/svs/runtime/ivf_index.h index 3bbe8d10b..37bb86477 100644 --- a/bindings/cpp/include/svs/runtime/ivf_index.h +++ b/bindings/cpp/include/svs/runtime/ivf_index.h @@ -24,8 +24,7 @@ namespace svs { namespace runtime { -namespace v0 { - +SVS_DECLARE_NAMESPACE_VERSION(0) { // Abstract interface for IVF (Inverted File) indices. struct SVS_RUNTIME_API IVFIndex { virtual ~IVFIndex(); @@ -170,7 +169,6 @@ struct SVS_RUNTIME_API IVFIndexLeanVec : public IVFIndex { size_t intra_query_threads = 1 ) noexcept; }; - -} // namespace v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) } // namespace runtime } // namespace svs diff --git a/bindings/cpp/include/svs/runtime/training.h b/bindings/cpp/include/svs/runtime/training.h index da072e42b..02c669dda 100644 --- a/bindings/cpp/include/svs/runtime/training.h +++ b/bindings/cpp/include/svs/runtime/training.h @@ -23,8 +23,7 @@ namespace svs { namespace runtime { -namespace v0 { - +SVS_DECLARE_NAMESPACE_VERSION(0) { struct SVS_RUNTIME_API LeanVecTrainingData { virtual ~LeanVecTrainingData(); @@ -70,7 +69,6 @@ struct SVS_RUNTIME_API LeanVecTrainingData { virtual Status save(std::ostream& out) const noexcept = 0; static Status load(LeanVecTrainingData** training_data, std::istream& in) noexcept; }; - -} // namespace v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) } // namespace runtime } // namespace svs diff --git a/bindings/cpp/include/svs/runtime/vamana_index.h b/bindings/cpp/include/svs/runtime/vamana_index.h index 2149c0d3e..680017619 100644 --- a/bindings/cpp/include/svs/runtime/vamana_index.h +++ b/bindings/cpp/include/svs/runtime/vamana_index.h @@ -23,8 +23,7 @@ namespace svs { namespace runtime { -namespace v0 { - +SVS_DECLARE_NAMESPACE_VERSION(0) { namespace detail { struct VamanaBuildParameters { size_t graph_max_degree = Unspecify(); @@ -128,7 +127,6 @@ struct SVS_RUNTIME_API VamanaIndexLeanVec : public VamanaIndex { const VamanaIndex::SearchParams& default_search_params = {} ) noexcept; }; - -} // namespace v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) } // namespace runtime } // namespace svs diff --git a/bindings/cpp/include/svs/runtime/version.h b/bindings/cpp/include/svs/runtime/version.h index bdf56450d..97b027fc6 100644 --- a/bindings/cpp/include/svs/runtime/version.h +++ b/bindings/cpp/include/svs/runtime/version.h @@ -21,7 +21,7 @@ /// /// This header defines the SVS Runtime API versioning scheme: /// 1. Versioned namespaces (e.g., v0, v1) for API stability -/// 2. Using declarations to bring current version to parent namespace +/// 2. Inline namespace to expose the current API version at svs::runtime::* /// 3. Clean integration points for external libraries /// /// Usage: @@ -59,21 +59,21 @@ #define SVS_RUNTIME_API_VERSION SVS_RUNTIME_VERSION_MAJOR #endif +#define SVS_API_VERSION_NS_IMPL(version) v##version +#define SVS_API_VERSION_NS(version) SVS_API_VERSION_NS_IMPL(version) +#define SVS_RUNTIME_API_VERSION_NAMESPACE SVS_API_VERSION_NS(SVS_RUNTIME_API_VERSION) + #if (SVS_RUNTIME_API_VERSION == 0) -/// Use v0 API -/// Current API version namespace -#define SVS_RUNTIME_CURRENT_API_NAMESPACE v0 -namespace svs { -namespace runtime { /// Current API version namespace (v0) /// All public runtime APIs live here and are accessible as svs::runtime::FunctionName /// due to inline namespace -inline namespace v0 { -// Public runtime APIs will be defined in their respective headers -// IMPORTANT: include this header before other runtime headers to ensure proper versioning -} -} // namespace runtime -} // namespace svs +#define SVS_DECLARE_NAMESPACE_VERSION_0 inline namespace SVS_API_VERSION_NS(0) + +// Forward declaration for the current version namespace as inline namespace +// This enforces compilation warnings/errors if the v0 namespace is not inlined below +namespace svs::runtime { +inline namespace v0 {} +} // namespace svs::runtime #else #error "Unsupported SVS Runtime major version" #endif @@ -86,11 +86,15 @@ inline namespace v0 { #define SVS_RUNTIME_CREATE_API_ALIAS(alias_name, version_ns) \ namespace alias_name = svs::runtime::version_ns +/// Helper macro to declare versioned namespaces for API definitions +#define SVS_DECLARE_NAMESPACE_VERSION_IMPL(version) SVS_DECLARE_NAMESPACE_VERSION_##version +#define SVS_DECLARE_NAMESPACE_VERSION(version) SVS_DECLARE_NAMESPACE_VERSION_IMPL(version) + /// /// @brief Version information structure for runtime queries /// -namespace svs::runtime::v0 { - +namespace svs::runtime { +SVS_DECLARE_NAMESPACE_VERSION(0) { struct VersionInfo { static constexpr int major = SVS_RUNTIME_VERSION_MAJOR; static constexpr int minor = SVS_RUNTIME_VERSION_MINOR; @@ -109,5 +113,5 @@ struct VersionInfo { return major == requested_major; } }; - -} // namespace svs::runtime::v0 +} // SVS_DECLARE_NAMESPACE_VERSION(0) +} // namespace svs::runtime diff --git a/bindings/cpp/src/dynamic_vamana_index.cpp b/bindings/cpp/src/dynamic_vamana_index.cpp index 0c1a6a890..e2399b5d1 100644 --- a/bindings/cpp/src/dynamic_vamana_index.cpp +++ b/bindings/cpp/src/dynamic_vamana_index.cpp @@ -63,7 +63,7 @@ struct DynamicVamanaIndexManagerBase : public DynamicVamanaIndex { }); } - size_t blocksize_bytes() const noexcept { return impl_->blocksize_bytes(); } + size_t blocksize_bytes() const noexcept override { return impl_->blocksize_bytes(); } Status remove_selected(size_t* num_removed, const IDFilter& selector) noexcept override { diff --git a/bindings/cpp/src/dynamic_vamana_index_impl.h b/bindings/cpp/src/dynamic_vamana_index_impl.h index 516a16539..3cc0a6116 100644 --- a/bindings/cpp/src/dynamic_vamana_index_impl.h +++ b/bindings/cpp/src/dynamic_vamana_index_impl.h @@ -65,6 +65,8 @@ class DynamicVamanaIndexImpl { } } + virtual ~DynamicVamanaIndexImpl() = default; + size_t size() const { return impl_ ? impl_->size() : 0; } size_t blocksize_bytes() const { return 1u << dynamic_index_params_.blocksize_exp; } diff --git a/bindings/cpp/src/vamana_index_impl.h b/bindings/cpp/src/vamana_index_impl.h index 550257d4f..44e440754 100644 --- a/bindings/cpp/src/vamana_index_impl.h +++ b/bindings/cpp/src/vamana_index_impl.h @@ -69,6 +69,8 @@ class VamanaIndexImpl { } } + virtual ~VamanaIndexImpl() = default; + size_t size() const { return impl_ ? get_impl()->size() : 0; } size_t dimensions() const { return dim_; } diff --git a/cmake/clang-tidy.cmake b/cmake/clang-tidy.cmake index 167f5b7c1..6df315e7a 100644 --- a/cmake/clang-tidy.cmake +++ b/cmake/clang-tidy.cmake @@ -17,18 +17,46 @@ ##### if(SVS_EXPERIMENTAL_CLANG_TIDY) - find_program(CLANG_TIDY_EXE NAMES clang-tidy-14 clang-tidy-13 clang-tidy-12 clang-tidy) + find_program(CLANG_TIDY_EXE + NAMES + clang-tidy-17 clang-tidy-18 clang-tidy + ) if(NOT CLANG_TIDY_EXE) message(WARNING "SVS_EXPERIMENTAL_CLANG_TIDY is ON but clang-tidy is not found!") set(CLANG_TIDY_COMMAND "" CACHE STRING "" FORCE) else() + # Walk up to find .clang-tidy when built standalone from bindings/cpp. + if(EXISTS "${CMAKE_SOURCE_DIR}/.clang-tidy") + set(SVS_CLANG_TIDY_CONFIG "${CMAKE_SOURCE_DIR}/.clang-tidy") + set(SVS_CLANG_TIDY_HEADER_FILTER "${CMAKE_SOURCE_DIR}/include/svs/.*") + elseif(EXISTS "${CMAKE_SOURCE_DIR}/../../.clang-tidy") + set(SVS_CLANG_TIDY_CONFIG "${CMAKE_SOURCE_DIR}/../../.clang-tidy") + set(SVS_CLANG_TIDY_HEADER_FILTER "${CMAKE_SOURCE_DIR}/include/svs/.*") + else() + message(FATAL_ERROR "SVS_EXPERIMENTAL_CLANG_TIDY is ON but no .clang-tidy was found") + endif() + set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--format-style=file" - "--config-file=${CMAKE_SOURCE_DIR}/.clang-tidy" - "--header-filter=${CMAKE_SOURCE_DIR}/include/svs/*" + "--config-file=${SVS_CLANG_TIDY_CONFIG}" + "--header-filter=${SVS_CLANG_TIDY_HEADER_FILTER}" + "--warnings-as-errors=clang-diagnostic-*,bugprone-use-after-move" + # Suppress noise from args (e.g. --gcc-toolchain) that affect link-time + # paths but are unused during clang-tidy's parse-only invocation. + "--extra-arg=-Wno-unused-command-line-argument" ) - message("Clang tidy command: ${CLANG_TIDY_COMMAND}") + + # Point clang-tidy at gcc's toolchain so it can find libstdc++ headers. + if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + get_filename_component(_svs_gcc_bin_dir "${CMAKE_CXX_COMPILER}" DIRECTORY) + get_filename_component(_svs_gcc_toolchain "${_svs_gcc_bin_dir}" DIRECTORY) + list(APPEND CLANG_TIDY_COMMAND + "--extra-arg=--gcc-toolchain=${_svs_gcc_toolchain}" + ) + endif() + + message(STATUS "Clang tidy command: ${CLANG_TIDY_COMMAND}") endif() endif() diff --git a/cmake/options.cmake b/cmake/options.cmake index d83b179a4..b02b6785c 100644 --- a/cmake/options.cmake +++ b/cmake/options.cmake @@ -201,14 +201,19 @@ if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "In endif() endif() -# Provide better diagnostics for broken templates. if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - target_compile_options( - svs_compile_options - INTERFACE - -fconcepts-diagnostics-depth=10 - -ftemplate-backtrace-limit=0 - ) + # Provide better diagnostics for broken templates. + # These flags are gcc-only; clang-tidy uses the clang frontend to parse the + # compile command and would error with "unknown argument", so skip them when + # clang-tidy is enabled. + if (NOT SVS_EXPERIMENTAL_CLANG_TIDY) + target_compile_options( + svs_compile_options + INTERFACE + -fconcepts-diagnostics-depth=10 + -ftemplate-backtrace-limit=0 + ) + endif() if (CMAKE_CXX_COMPILER_VERSION GREATER_EQUAL 12.0) # GCC-12 throws errors in its own intrinsics library with uninitialized variables. diff --git a/docker/x86_64/manylinux228/Dockerfile b/docker/x86_64/manylinux228/Dockerfile index 0818982e2..491ca196e 100644 --- a/docker/x86_64/manylinux228/Dockerfile +++ b/docker/x86_64/manylinux228/Dockerfile @@ -24,6 +24,15 @@ RUN yum install -y cmake-3.26.5 \ RUN yum install -y gcc-toolset-11-11.1-1.el8 wget-1.19.5 \ && yum clean all +# Install clang-tidy via the LLVM 17 module; newer versions crash on our templates. +# The base image ships LLVM 21.x by default, so remove it first to ensure the +# module's older binary wins on PATH. +RUN yum remove -y clang-tools-extra clang || true \ + && dnf module reset -y llvm-toolset \ + && dnf module install -y llvm-toolset:rhel8 \ + && yum install -y clang-tools-extra \ + && yum clean all + # Configure environment setup properly without recursion RUN echo '# Configure gcc-11' > /etc/profile.d/01-gcc.sh && \ echo 'source scl_source enable gcc-toolset-11' >> /etc/profile.d/01-gcc.sh && \ diff --git a/include/svs/lib/threads/thread.h b/include/svs/lib/threads/thread.h index 5a5776ce5..6de938d3e 100644 --- a/include/svs/lib/threads/thread.h +++ b/include/svs/lib/threads/thread.h @@ -888,7 +888,7 @@ template class ThreadImpl { // * Catch this exception and wrap its message inside a `ThreadError`. try { unsafe_assign(fn); - } catch (const ThreadCrashedError& err) { + } catch (const ThreadCrashedError&) { try { unsafe_get_exception(); } catch (const std::exception& inner_error) { throw ThreadError{inner_error}; }