Skip to content

Commit

Permalink
[clang-tidy] Add --enable-module-headers-parsing option
Browse files Browse the repository at this point in the history
Change default behavior in Clang-tidy and skip by default
module headers parsing. That functionality is buggy and
slow in C++20, and provide tiny benefit.

Reviewed By: carlosgalvezp

Differential Revision: https://reviews.llvm.org/D156161
  • Loading branch information
PiotrZSL committed Jul 25, 2023
1 parent 24567dd commit b530eee
Show file tree
Hide file tree
Showing 7 changed files with 157 additions and 116 deletions.
3 changes: 2 additions & 1 deletion clang-tools-extra/clang-tidy/ClangTidy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,8 @@ ClangTidyASTConsumerFactory::createASTConsumer(
Preprocessor *PP = &Compiler.getPreprocessor();
Preprocessor *ModuleExpanderPP = PP;

if (Context.getLangOpts().Modules && OverlayFS != nullptr) {
if (Context.canEnableModuleHeadersParsing() &&
Context.getLangOpts().Modules && OverlayFS != nullptr) {
auto ModuleExpander = std::make_unique<ExpandModularHeadersPPCallbacks>(
&Compiler, OverlayFS);
ModuleExpanderPP = ModuleExpander->getPreprocessor();
Expand Down
3 changes: 2 additions & 1 deletion clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,11 @@ ClangTidyError::ClangTidyError(StringRef CheckName,

ClangTidyContext::ClangTidyContext(
std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
bool AllowEnablingAnalyzerAlphaCheckers)
bool AllowEnablingAnalyzerAlphaCheckers, bool EnableModuleHeadersParsing)
: DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
Profile(false),
AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers),
EnableModuleHeadersParsing(EnableModuleHeadersParsing),
SelfContainedDiags(false) {
// Before the first translation unit we can get errors related to command-line
// parsing, use empty string for the file name in this case.
Expand Down
10 changes: 9 additions & 1 deletion clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ class ClangTidyContext {
public:
/// Initializes \c ClangTidyContext instance.
ClangTidyContext(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
bool AllowEnablingAnalyzerAlphaCheckers = false);
bool AllowEnablingAnalyzerAlphaCheckers = false,
bool EnableModuleHeadersParsing = false);
/// Sets the DiagnosticsEngine that diag() will emit diagnostics to.
// FIXME: this is required initialization, and should be a constructor param.
// Fix the context -> diag engine -> consumer -> context initialization cycle.
Expand Down Expand Up @@ -198,6 +199,12 @@ class ClangTidyContext {
return AllowEnablingAnalyzerAlphaCheckers;
}

// This method determines whether preprocessor-level module header parsing is
// enabled using the `--experimental-enable-module-headers-parsing` option.
bool canEnableModuleHeadersParsing() const {
return EnableModuleHeadersParsing;
}

void setSelfContainedDiags(bool Value) { SelfContainedDiags = Value; }

bool areDiagsSelfContained() const { return SelfContainedDiags; }
Expand Down Expand Up @@ -245,6 +252,7 @@ class ClangTidyContext {
std::string ProfilePrefix;

bool AllowEnablingAnalyzerAlphaCheckers;
bool EnableModuleHeadersParsing;

bool SelfContainedDiags;

Expand Down
14 changes: 13 additions & 1 deletion clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,17 @@ static cl::opt<bool>
cl::init(false), cl::Hidden,
cl::cat(ClangTidyCategory));

static cl::opt<bool> EnableModuleHeadersParsing("enable-module-headers-parsing",
desc(R"(
Enables preprocessor-level module header parsing
for C++20 and above, empowering specific checks
to detect macro definitions within modules. This
feature may cause performance and parsing issues
and is therefore considered experimental.
)"),
cl::init(false),
cl::cat(ClangTidyCategory));

static cl::opt<std::string> ExportFixes("export-fixes", desc(R"(
YAML file to store suggested fixes in. The
stored fixes can be applied to the input source
Expand Down Expand Up @@ -659,7 +670,8 @@ int clangTidyMain(int argc, const char **argv) {
llvm::InitializeAllAsmParsers();

ClangTidyContext Context(std::move(OwningOptionsProvider),
AllowEnablingAnalyzerAlphaCheckers);
AllowEnablingAnalyzerAlphaCheckers,
EnableModuleHeadersParsing);
std::vector<ClangTidyError> Errors =
runClangTidy(Context, OptionsParser->getCompilations(), PathList, BaseFS,
FixNotes, EnableCheckProfile, ProfilePrefix);
Expand Down
14 changes: 14 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,20 @@ The improvements are...
Improvements to clang-tidy
--------------------------

- Preprocessor-level module header parsing is now disabled by default due to
the problems it caused in C++20 and above, leading to performance and code
parsing issues regardless of whether modules were used or not. This change
will impact only the following checks:
:doc:`modernize-replace-disallow-copy-and-assign-macro
<clang-tidy/checks/modernize/replace-disallow-copy-and-assign-macro>`,
:doc:`bugprone-reserved-identifier
<clang-tidy/checks/bugprone/reserved-identifier>`, and
:doc:`readability-identifier-naming
<clang-tidy/checks/readability/identifier-naming>`. Those checks will no
longer see macros defined in modules. Users can still enable this
functionality using the newly added command line option
`--enable-module-headers-parsing`.

New checks
^^^^^^^^^^

Expand Down

0 comments on commit b530eee

Please sign in to comment.