Skip to content

Conversation

@slackito
Copy link
Collaborator

The original commit removed them to avoid the dependency on llvm Support because of LLVM_FALLTHROUGH, but this triggers -Wimplicit-fallthrough warnings.

LLVM requires a C++17 compiler now, so we should be able to use the standard [[fallthrough]] attribute.

The original commit removed them to avoid the dependency on llvm Support
because of LLVM_FALLTHROUGH, but this triggers `-Wimplicit-fallthrough`
warnings.

LLVM requires a C++17 compiler now, so we should be able to use the
standard [[fallthrough]] attribute.
@slackito slackito requested a review from pcc July 10, 2025 18:56
@slackito slackito merged commit 00dd666 into llvm:main Jul 10, 2025
9 of 10 checks passed
@slackito slackito deleted the siphash-fallthrough branch July 10, 2025 19:03
@pcc
Copy link
Contributor

pcc commented Jul 10, 2025

So can we remove the LLVM_FALLTHROUGH macro entirely now?

@slackito
Copy link
Collaborator Author

The LLVM Coding Standards doc says

Unless otherwise documented, LLVM subprojects are written using standard C++17 code and avoid unnecessary vendor-specific extensions.

Nevertheless, we restrict ourselves to features which are available in the major toolchains supported as host compilers (see Getting Started with the LLVM System page, section Software).

And that Software section in the Getting Started document says:

LLVM is written using the subset of C++ documented in coding standards. To enforce this language version, we check the most popular host toolchains for specific minimum versions in our build systems:

  • Clang 5.0
  • Apple Clang 10.0
  • GCC 7.4
  • Visual Studio 2019 16.8

According to https://en.cppreference.com/w/cpp/compiler_support/17, [[fallthrough]] is supported by GCC 7, Clang 3.9, VS 2017 15.0 and Apple Clang. So I'd say yes we can. But that's way out of scope for this PR.

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