[RFC] Expand Modular Headers PPCallbacks problem in C++20

Clang Frontend clang-tidy
1

Issue looks like this, to support preprocessor handling in Clang-tidy when modules are in use, in 2019 there were added class ExpandModularHeadersPPCallbacks.

commit bbc89dcb29035ad450ac0981b8f51a6cfa2aa8ba
Author: Alexander Kornienko <alexfh@google.com>
Date:   Fri Mar 22 13:42:48 2019 +0000

    [clang-tidy] Expand modular headers for PPCallbacks
    ...
    Differential Revision: https://reviews.llvm.org/D59528

When C++20 & Modules are enabled Clang-Tidy create second instance of Preprocessor and parse input files all over again including ones from modules.

This creates 2 issues, performance degradation that were reported in ([clang-tidy] Performance regression when enabling c++20 · Issue #62337 · llvm/llvm-project · GitHub), and invalid handling of built-in functions due to some differences between original preprocessor and one created later (clang-tidy gives incorrect result for `__has_builtin` · Issue #59807 · llvm/llvm-project · GitHub, [clang-tidy] __has_builtin mishandled when using modules · Issue #50087 · llvm/llvm-project · GitHub).

Main problem is that those issues (performance degradation + __has_builtin happens even if project do not use modules, but enabled C++20. Currently, when more projects switch into C++20, this problem become more and more visible.

There are only 3 checks relay on this class:

  • modernize-replace-disallow-copy-and-assign-macro
  • bugprone-reserved-identifier
  • readability-identifier-naming

Best would be to re-implement this functionality without creating additional Preprocessor & parsing files all over again (is there anyone who can do that ?). But as Clang 17 is close (~1 month to branch out), options would be:

a) Add option --enable-module-headers-parsing (or other name), and disable this functionality by default, and only enable when this option would be provided.
b) Add option --disable-module-headers-parsing (or other name), and leave this functionality ON by default, and only disable when this option would be provided.
c) Remove this functionality completely, and assume that it should be handled by Clang, not Clang-tidy.

I would probably choose a), but what is your opinion ?

1 Like
2

~2 days till Clang-tidy 17 branch out.
I will go forward with ⚙ D156024 [clang-tidy] Add --disable-modular-headers-expansion option (option B) before branch out (just to enable some workaround for users). But still I believe that proper way would be simply to remove this functionality (option C) as it breaks handling of C++20 and above in Clang-tidy.

Next known issue is [clang-tidy] LLVM 16 will promote suppressed warnings to errors when C++20 is enabled · Issue #61969 · llvm/llvm-project · GitHub
We are unable to disable macro-specific warnings in clang-tidy.

3

Personally I think it would be good with option a), perhaps call it like they do in Bazel with the --experimental prefix, like --experimental-enable-module-headers-parsing or similar, to communicate that this might enable some features, but break others (like it does). The only issue with either a) or b) is that they are temporary things that we need to eventually deprecate, and that takes 1 year to do :slight_smile:

Alternatively we could revert the patch entirely, but I don’t feel knowledgeable enough to determine if that’s the best path (or even doable 4 years after it was merged). It would be great to hear from @alexfh (author) or @gribozavr (reviewer) for an expert opinion.