[CPPRuntime] Fix C++ Runtime Clang compilation errors#328
Conversation
… cpp runtime bindings
2548516 to
d3ef2c8
Compare
…for versioning consistency
Co-Authored-By: mnorris11 <mnorris@meta.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the C++ runtime bindings to compile cleanly under Clang by standardizing API versioned namespaces via macros, and extends CI + build tooling to exercise both GCC and Clang builds.
Changes:
- Introduces
SVS_DECLARE_NAMESPACE_VERSIONto define versioned inline namespaces across runtime public headers. - Extends CI/build scripts and the manylinux Docker image to support selecting GCC vs Clang.
- Improves runtime implementation correctness by adding
overrideand virtual destructors where polymorphic deletion can occur.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/x86_64/manylinux228/Dockerfile | Installs Clang in the manylinux build image used by CI. |
| bindings/cpp/src/vamana_index_impl.h | Adds a virtual destructor to a base implementation class used via inheritance. |
| bindings/cpp/src/dynamic_vamana_index.cpp | Adds missing override on an interface implementation method. |
| bindings/cpp/src/dynamic_vamana_index_impl.h | Adds a virtual destructor to a base implementation class used via inheritance. |
| bindings/cpp/include/svs/runtime/version.h | Adds macro-based namespace versioning helpers and adjusts VersionInfo to live in the versioned namespace. |
| bindings/cpp/include/svs/runtime/vamana_index.h | Switches from namespace v0 to SVS_DECLARE_NAMESPACE_VERSION(0). |
| bindings/cpp/include/svs/runtime/training.h | Switches from namespace v0 to SVS_DECLARE_NAMESPACE_VERSION(0). |
| bindings/cpp/include/svs/runtime/ivf_index.h | Switches from namespace v0 to SVS_DECLARE_NAMESPACE_VERSION(0). |
| bindings/cpp/include/svs/runtime/flat_index.h | Switches from namespace v0 to SVS_DECLARE_NAMESPACE_VERSION(0). |
| bindings/cpp/include/svs/runtime/dynamic_vamana_index.h | Switches to SVS_DECLARE_NAMESPACE_VERSION(0) and removes redundant redeclarations already present in VamanaIndex. |
| bindings/cpp/include/svs/runtime/dynamic_ivf_index.h | Switches from namespace v0 to SVS_DECLARE_NAMESPACE_VERSION(0). |
| bindings/cpp/include/svs/runtime/api_defs.h | Switches from namespace v0 to SVS_DECLARE_NAMESPACE_VERSION(0) for common runtime types. |
| bindings/cpp/CMakeLists.txt | Adds a Clang-only warning intended to catch inline-namespace misuse. |
| .github/workflows/build-cpp-runtime-bindings.yml | Adds compiler selection (GCC/Clang) to the workflow matrix and passes CC/CXX into the container. |
| .github/scripts/build-cpp-runtime-bindings.sh | Uses CC/CXX from the environment (with sensible defaults) when configuring CMake. |
| .clang-format | Treats SVS_DECLARE_NAMESPACE_VERSION as a namespace macro for formatting. |
5683b73 to
322d1ca
Compare
| cc: "gcc" | ||
| cxx: "g++" | ||
| - name: public only with Clang | ||
| enable_lvq_leanvec: "OFF" |
There was a problem hiding this comment.
Is faiss requesting a clang build? Without lvq/leanvec support?
There was a problem hiding this comment.
I do not know, but there were Clang compilation errors for CPP Runtime.
This why I added the option to compile runtime with Clang. The "public-only" case should be enough to catch Clang compilation issues.
See #324 for more details.
There was a problem hiding this comment.
@ethanglaser, can you please suggest the proper way to validate SVS CPP Runtime sources for Clang and/or Intel ICPX compatibility in CI?
Thank you.
There was a problem hiding this comment.
Based on the info from Michael we can remove the changes to the Dockerfile and .github workflow as we will not need to validate compiling runtime bindings with clang
There was a problem hiding this comment.
Working on an update to this based on the clang-tidy approach: #332, still working through some errors there
This pull request introduces C++ API versioned namespaces using a macro-based approach, improving maintainability and future-proofing the API. It also adds support for building and testing with both GCC and Clang compilers in CI, and includes several related build and code quality improvements.
Note: Changes in PR are based on #324 made by @mnorris11
API Versioning and Namespace Refactoring:
SVS_DECLARE_NAMESPACE_VERSIONmacro for versioned inline namespaces in the runtime API, replacing manualnamespace v0 { ... }blocks in all public headers. This change standardizes versioning and makes it easier to add new API versions in the future. (bindings/cpp/include/svs/runtime/version.h,bindings/cpp/include/svs/runtime/api_defs.h,bindings/cpp/include/svs/runtime/dynamic_ivf_index.h,bindings/cpp/include/svs/runtime/dynamic_vamana_index.h,bindings/cpp/include/svs/runtime/flat_index.h,bindings/cpp/include/svs/runtime/ivf_index.h,bindings/cpp/include/svs/runtime/training.h,bindings/cpp/include/svs/runtime/vamana_index.h) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]VersionInfostructure and related macros to use the new namespace versioning scheme. (bindings/cpp/include/svs/runtime/version.h) [1] [2]Compiler and Build System Improvements:
.github/workflows/build-cpp-runtime-bindings.yml,.github/scripts/build-cpp-runtime-bindings.sh,docker/x86_64/manylinux228/Dockerfile) [1] [2] [3] [4]bindings/cpp/CMakeLists.txt)Code Quality and Maintenance:
overrideand virtual destructors to implementation classes for better memory safety and clarity. (bindings/cpp/src/dynamic_vamana_index.cpp,bindings/cpp/src/dynamic_vamana_index_impl.h,bindings/cpp/src/vamana_index_impl.h) [1] [2] [3]Formatting and Linting:
.clang-formatto recognize the newSVS_DECLARE_NAMESPACE_VERSIONmacro as a namespace macro. (.clang-format)