Skip to content

fix(build): avoid digit separators in expected preprocessor checks - #1098

Open
wgtmac wants to merge 1 commit into
apache:mainfrom
wgtmac:codex/fix-expected-dependency-scanning
Open

wgtmac wants to merge 1 commit into
apache:mainfrom
wgtmac:codex/fix-expected-dependency-scanning

Conversation

@wgtmac

@wgtmac wgtmac commented Nov 6, 2026

Copy link
Copy Markdown
Member

LLVM 21.1.8's clang-scan-deps rejects digit separators in the #if expressions in compat/expected.h, even though ordinary Clang compilation accepts them. This breaks downstream CMake module dependency scanning with -format=p1689:

error: token is not a valid binary operator in a preprocessor subexpression

Use plain 201703L and 202002L literals in all nine language-version checks. The values and conditional behavior are unchanged.

Validation with LLVM 21.1.8:

  • Reproduced nine dependency-scanning errors before the change in both C++23 and C++20 compatibility mode; the same default scanner succeeds after the change.
  • Compiled and ran the existing expected_test.cc suite in both modes: 30 tests passed in each.
  • pre-commit and git diff --check passed.

Copilot AI balanced review requested due to automatic review settings November 6, 2026 09:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues are identified for the compatibility-header change.

No open issues 🎉

What changed in this PR

This PR makes the expected compatibility header compatible with LLVM 21.1.8 dependency scanning without changing its language-version conditions.

Changes:

  • Removes digit separators from all nine preprocessor language-version checks.
  • Preserves the C++17 and C++20 thresholds using 201703L and 202002L.
File Description
src/​iceberg/​compat/​expected.h Uses plain integer literals in preprocessor checks to avoid clang-scan-deps parsing errors.

Review effort: Balanced

@zhjwpku zhjwpku left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This branch has not been deployed

No deployments
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