Skip to content

build: support config clang stdlib and fix default#210

Merged
feihongxu0824 merged 9 commits intomainfrom
fix-clang-link
Mar 11, 2026
Merged

build: support config clang stdlib and fix default#210
feihongxu0824 merged 9 commits intomainfrom
fix-clang-link

Conversation

@egolearner
Copy link
Collaborator

@egolearner egolearner commented Mar 10, 2026

Greptile Summary

This PR introduces a user-configurable CLANG_USE_LIBCXX CMake option to control whether Clang uses libc++ or libstdc++, replacing the previous hard-coded -stdlib=libc++ flag. The default is ON on Apple platforms and OFF elsewhere, which aligns with platform conventions. The change also cleans up trailing whitespace in several function bodies.

Key changes:

  • New CLANG_USE_LIBCXX option and derived CLANG_STDLIB_OPTION variable used across compile and link flags.
  • Compile-flag generator expression and both strict/unstrict link-flag entries updated to use the variable.
  • Whitespace-only cleanup throughout _target_link_libraries and helpers.

Issues found:

  • When CLANG_STDLIB_OPTION is empty, the compile-flag generator expression gains a trailing semicolon (-fcolor-diagnostics;), which causes CMake to emit an empty string as a separate compiler argument — a potential build error on some Clang versions.
  • CMAKE_CXX_COMPILER_ID MATCHES "Clang" uses regex and inadvertently matches "AppleClang". Because the downstream generator expressions use exact CXX_COMPILER_ID:Clang matching, CLANG_STDLIB_OPTION is set for Apple Clang users but silently never applied. Using STREQUAL "Clang" would make the intent explicit.
  • The -stdlib flag is forwarded to C compilations (via C_COMPILER_ID:Clang), which is unnecessary since -stdlib is a C++ concept.

Confidence Score: 3/5

  • The PR is mostly safe to merge but contains a generator-expression bug that can inject a blank compiler argument on Linux with LLVM Clang when libc++ is not selected.
  • The core intent is sound and defaults are platform-appropriate. However, the empty-CLANG_STDLIB_OPTION trailing-semicolon issue can cause silent or noisy build failures for Linux/LLVM Clang users with the default OFF setting, and the MATCHES vs STREQUAL inconsistency makes the logic harder to maintain. These issues reduce confidence below a clean pass.
  • cmake/bazel.cmake — generator expression at line 389 and the MATCHES condition at line 375 both need attention.

Important Files Changed

Filename Overview
cmake/bazel.cmake Adds a configurable CLANG_USE_LIBCXX option (default ON on Apple, OFF elsewhere) and a CLANG_STDLIB_OPTION variable used in compile/link flags. Key issues: empty CLANG_STDLIB_OPTION creates a trailing semicolon in the generator expression that can emit a blank compiler argument; MATCHES "Clang" unintentionally catches AppleClang while downstream generator expressions do not; -stdlib flag is unnecessarily applied to C compilations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CMake Configure] --> B{APPLE platform?}
    B -- Yes --> C[CLANG_USE_LIBCXX = ON default]
    B -- No --> D[CLANG_USE_LIBCXX = OFF default]
    C --> E{CMAKE_CXX_COMPILER_ID\nMATCHES Clang?\nnote: also matches AppleClang}
    D --> E
    E -- No --> F[CLANG_STDLIB_OPTION = empty string]
    E -- Yes --> G{CLANG_USE_LIBCXX?}
    G -- ON --> H[CLANG_STDLIB_OPTION = -stdlib=libc++]
    G -- OFF --> I{APPLE?}
    I -- Yes --> F
    I -- No --> J[CLANG_STDLIB_OPTION = -stdlib=libstdc++]
    H --> K[Applied via generator expressions\nC_COMPILER_ID:Clang compile flags\nCXX_COMPILER_ID:Clang link flags]
    J --> K
    F --> L[⚠️ Empty value → trailing semicolon\nin generator expression → blank arg]
    K --> M[Build targets use CLANG_STDLIB_OPTION]
Loading

Last reviewed commit: a217750

Greptile also left 3 inline comments on this PR.

@feihongxu0824 feihongxu0824 merged commit 6b89d97 into main Mar 11, 2026
9 checks passed
@egolearner egolearner deleted the fix-clang-link branch March 11, 2026 08:34
@egolearner
Copy link
Collaborator Author

#214 add clang CI and will track ut problem there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants