[clang][AST][clang-tidy] Do not set a reduced traversal scope in ASTM… #132725
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…atchFinder This was introduced here with the purpose of speed up clang-tidy, making it not process system headers: llvm@e4a8969 However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks: llvm#128150 (comment) This patch fixes this problem, as follows: - Revert the changes to the clang-tidy unit tests. - Revert setting the TraversalScope in MatchASTConsumer. - Move the logic to MatchASTVisitor::TraverseDecl instead. Pros: - Leaves the ASTContext and ParentMapContext untouched, so hasParent and similar matchers should work again. - Keeps avoiding matching and checking decls outside of system headers. - The broken unit test in readability now works, because we are no longer processing only TopLevelDecls. - The changes to the CERT test can be reverted. - Most likely we don't lose any functionality or introduce false negatives. Cons: - We still have to traverse decls in system headers. I did try to return false; instead of return true; in the proposed code but that made around 60 clang-tidy tests to fail. - We still process many nodes (not Decls) that are in system headers. As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to: * Baseline (without any optimizations). Suppressed 196093 warnings (196093 in non-user code) 10 seconds to run. * Trunk (aggressive optimizations). Suppressed 8050 warnings (8050 in non-user code). 1 second to run. * This patch (conservative optimizations). Suppressed 141779 warnings (141779 in non-user code). 3 seconds to run. This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy. Fixes llvm#130618
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang Author: Carlos Galvez (carlosgalvezp) Changes…atchFinder This was introduced here with the purpose of speed up clang-tidy, making it not process system headers: However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks: This patch fixes this problem, as follows:
Pros:
Cons:
As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to:
This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy. Fixes #130618 Full diff: https://github.com/llvm/llvm-project/pull/132725.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
index 2dff4c0e53b8c..bc4970825b4ca 100644
--- a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
@@ -35,30 +35,6 @@ AST_POLYMORPHIC_MATCHER_P(
Builder) != Args.end();
}
-bool isStdOrPosixImpl(const DeclContext *Ctx) {
- if (!Ctx->isNamespace())
- return false;
-
- const auto *ND = cast<NamespaceDecl>(Ctx);
- if (ND->isInline()) {
- return isStdOrPosixImpl(ND->getParent());
- }
-
- if (!ND->getParent()->getRedeclContext()->isTranslationUnit())
- return false;
-
- const IdentifierInfo *II = ND->getIdentifier();
- return II && (II->isStr("std") || II->isStr("posix"));
-}
-
-AST_MATCHER(Decl, isInStdOrPosixNS) {
- for (const auto *Ctx = Node.getDeclContext(); Ctx; Ctx = Ctx->getParent()) {
- if (isStdOrPosixImpl(Ctx))
- return true;
- }
- return false;
-}
-
} // namespace
namespace clang::tidy::cert {
@@ -66,10 +42,12 @@ namespace clang::tidy::cert {
void DontModifyStdNamespaceCheck::registerMatchers(MatchFinder *Finder) {
auto HasStdParent =
hasDeclContext(namespaceDecl(hasAnyName("std", "posix"),
- unless(hasDeclContext(namespaceDecl())))
+ unless(hasParent(namespaceDecl())))
.bind("nmspc"));
- auto UserDefinedType = qualType(hasUnqualifiedDesugaredType(
- tagType(unless(hasDeclaration(tagDecl(isInStdOrPosixNS()))))));
+ auto UserDefinedType = qualType(
+ hasUnqualifiedDesugaredType(tagType(unless(hasDeclaration(tagDecl(
+ hasAncestor(namespaceDecl(hasAnyName("std", "posix"),
+ unless(hasParent(namespaceDecl()))))))))));
auto HasNoProgramDefinedTemplateArgument = unless(
hasAnyTemplateArgumentIncludingPack(refersToType(UserDefinedType)));
auto InsideStdClassOrClassTemplateSpecialization = hasDeclContext(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ed7da975f3de7..a05be39c8dce5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,8 +94,6 @@ Improvements to clang-tidy
- :program:`clang-tidy` no longer processes declarations from system headers
by default, greatly improving performance. This behavior is disabled if the
`SystemHeaders` option is enabled.
- Note: this may lead to false negatives; downstream users may need to adjust
- their checks to preserve existing behavior.
- Improved :program:`clang-tidy-diff.py` script. Add the `-warnings-as-errors`
argument to treat warnings as errors.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
index ad6525276ff8a..1b4d4e924a721 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
@@ -33,29 +33,23 @@
// RUN: readability-identifier-naming.LocalConstantPointerPrefix: 'lc_', \
// RUN: }}'
-// FIXME: make this test case pass.
-// Currently not working because the CXXRecordDecl for the global anonymous
-// union is *not* collected as a top-level declaration.
-// https://github.com/llvm/llvm-project/issues/130618
-#if 0
static union {
int global;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
-// FIXME-CHECK-FIXES: {{^}} int g_global;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
+// CHECK-FIXES: {{^}} int g_global;{{$}}
const int global_const;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
-// FIXME-CHECK-FIXES: {{^}} const int GLOBAL_CONST;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
+// CHECK-FIXES: {{^}} const int GLOBAL_CONST;{{$}}
int *global_ptr;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
-// FIXME-CHECK-FIXES: {{^}} int *GlobalPtr_Ptr;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
+// CHECK-FIXES: {{^}} int *GlobalPtr_Ptr;{{$}}
int *const global_const_ptr;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
-// FIXME-CHECK-FIXES: {{^}} int *const GLOBAL_CONST_PTR_Ptr;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
+// CHECK-FIXES: {{^}} int *const GLOBAL_CONST_PTR_Ptr;{{$}}
};
-#endif
namespace ns {
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8182bccdd2da8..f4698b6c70c81 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -460,9 +460,8 @@ AST Matchers
specialization.
- Move ``ast_matchers::MatchFinder::MatchFinderOptions`` to
``ast_matchers::MatchFinderOptions``.
-- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``, and make
- ``MatchASTConsumer`` receive a reference to ``MatchFinderOptions`` in the
- constructor. This allows it to skip system headers when traversing the AST.
+- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``. This
+ allows it to avoid matching declarations in system headers .
clang-format
------------
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index e347d0c54d9b0..995d9986b1ba3 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -28,7 +28,6 @@
#include <deque>
#include <memory>
#include <set>
-#include <vector>
namespace clang {
namespace ast_matchers {
@@ -1464,11 +1463,21 @@ bool MatchASTVisitor::objcClassIsDerivedFrom(
return false;
}
+static bool isInSystemHeader(Decl *D) {
+ const SourceManager &SM = D->getASTContext().getSourceManager();
+ const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
+ return SM.isInSystemHeader(Loc);
+}
+
bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
if (!DeclNode) {
return true;
}
+ // Skip declarations in system headers if requested
+ if (Options.SkipSystemHeaders && isInSystemHeader(DeclNode))
+ return true;
+
bool ScopedTraversal =
TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
bool ScopedChildren = TraversingASTChildrenNotSpelledInSource;
@@ -1574,41 +1583,19 @@ bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) {
class MatchASTConsumer : public ASTConsumer {
public:
MatchASTConsumer(MatchFinder *Finder,
- MatchFinder::ParsingDoneTestCallback *ParsingDone,
- const MatchFinderOptions &Options)
- : Finder(Finder), ParsingDone(ParsingDone), Options(Options) {}
+ MatchFinder::ParsingDoneTestCallback *ParsingDone)
+ : Finder(Finder), ParsingDone(ParsingDone) {}
private:
- bool HandleTopLevelDecl(DeclGroupRef DG) override {
- if (Options.SkipSystemHeaders) {
- for (Decl *D : DG) {
- if (!isInSystemHeader(D))
- TraversalScope.push_back(D);
- }
- }
- return true;
- }
-
void HandleTranslationUnit(ASTContext &Context) override {
- if (!TraversalScope.empty())
- Context.setTraversalScope(TraversalScope);
-
if (ParsingDone != nullptr) {
ParsingDone->run();
}
Finder->matchAST(Context);
}
- bool isInSystemHeader(Decl *D) {
- const SourceManager &SM = D->getASTContext().getSourceManager();
- const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
- return SM.isInSystemHeader(Loc);
- }
-
MatchFinder *Finder;
MatchFinder::ParsingDoneTestCallback *ParsingDone;
- const MatchFinderOptions &Options;
- std::vector<Decl *> TraversalScope;
};
} // end namespace
@@ -1727,8 +1714,7 @@ bool MatchFinder::addDynamicMatcher(const internal::DynTypedMatcher &NodeMatch,
}
std::unique_ptr<ASTConsumer> MatchFinder::newASTConsumer() {
- return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone,
- Options);
+ return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone);
}
void MatchFinder::match(const clang::DynTypedNode &Node, ASTContext &Context) {
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Carlos Galvez (carlosgalvezp) Changes…atchFinder This was introduced here with the purpose of speed up clang-tidy, making it not process system headers: However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks: This patch fixes this problem, as follows:
Pros:
Cons:
As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to:
This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy. Fixes #130618 Full diff: https://github.com/llvm/llvm-project/pull/132725.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
index 2dff4c0e53b8c..bc4970825b4ca 100644
--- a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
@@ -35,30 +35,6 @@ AST_POLYMORPHIC_MATCHER_P(
Builder) != Args.end();
}
-bool isStdOrPosixImpl(const DeclContext *Ctx) {
- if (!Ctx->isNamespace())
- return false;
-
- const auto *ND = cast<NamespaceDecl>(Ctx);
- if (ND->isInline()) {
- return isStdOrPosixImpl(ND->getParent());
- }
-
- if (!ND->getParent()->getRedeclContext()->isTranslationUnit())
- return false;
-
- const IdentifierInfo *II = ND->getIdentifier();
- return II && (II->isStr("std") || II->isStr("posix"));
-}
-
-AST_MATCHER(Decl, isInStdOrPosixNS) {
- for (const auto *Ctx = Node.getDeclContext(); Ctx; Ctx = Ctx->getParent()) {
- if (isStdOrPosixImpl(Ctx))
- return true;
- }
- return false;
-}
-
} // namespace
namespace clang::tidy::cert {
@@ -66,10 +42,12 @@ namespace clang::tidy::cert {
void DontModifyStdNamespaceCheck::registerMatchers(MatchFinder *Finder) {
auto HasStdParent =
hasDeclContext(namespaceDecl(hasAnyName("std", "posix"),
- unless(hasDeclContext(namespaceDecl())))
+ unless(hasParent(namespaceDecl())))
.bind("nmspc"));
- auto UserDefinedType = qualType(hasUnqualifiedDesugaredType(
- tagType(unless(hasDeclaration(tagDecl(isInStdOrPosixNS()))))));
+ auto UserDefinedType = qualType(
+ hasUnqualifiedDesugaredType(tagType(unless(hasDeclaration(tagDecl(
+ hasAncestor(namespaceDecl(hasAnyName("std", "posix"),
+ unless(hasParent(namespaceDecl()))))))))));
auto HasNoProgramDefinedTemplateArgument = unless(
hasAnyTemplateArgumentIncludingPack(refersToType(UserDefinedType)));
auto InsideStdClassOrClassTemplateSpecialization = hasDeclContext(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ed7da975f3de7..a05be39c8dce5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,8 +94,6 @@ Improvements to clang-tidy
- :program:`clang-tidy` no longer processes declarations from system headers
by default, greatly improving performance. This behavior is disabled if the
`SystemHeaders` option is enabled.
- Note: this may lead to false negatives; downstream users may need to adjust
- their checks to preserve existing behavior.
- Improved :program:`clang-tidy-diff.py` script. Add the `-warnings-as-errors`
argument to treat warnings as errors.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
index ad6525276ff8a..1b4d4e924a721 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
@@ -33,29 +33,23 @@
// RUN: readability-identifier-naming.LocalConstantPointerPrefix: 'lc_', \
// RUN: }}'
-// FIXME: make this test case pass.
-// Currently not working because the CXXRecordDecl for the global anonymous
-// union is *not* collected as a top-level declaration.
-// https://github.com/llvm/llvm-project/issues/130618
-#if 0
static union {
int global;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
-// FIXME-CHECK-FIXES: {{^}} int g_global;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
+// CHECK-FIXES: {{^}} int g_global;{{$}}
const int global_const;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
-// FIXME-CHECK-FIXES: {{^}} const int GLOBAL_CONST;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
+// CHECK-FIXES: {{^}} const int GLOBAL_CONST;{{$}}
int *global_ptr;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
-// FIXME-CHECK-FIXES: {{^}} int *GlobalPtr_Ptr;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
+// CHECK-FIXES: {{^}} int *GlobalPtr_Ptr;{{$}}
int *const global_const_ptr;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
-// FIXME-CHECK-FIXES: {{^}} int *const GLOBAL_CONST_PTR_Ptr;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
+// CHECK-FIXES: {{^}} int *const GLOBAL_CONST_PTR_Ptr;{{$}}
};
-#endif
namespace ns {
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8182bccdd2da8..f4698b6c70c81 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -460,9 +460,8 @@ AST Matchers
specialization.
- Move ``ast_matchers::MatchFinder::MatchFinderOptions`` to
``ast_matchers::MatchFinderOptions``.
-- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``, and make
- ``MatchASTConsumer`` receive a reference to ``MatchFinderOptions`` in the
- constructor. This allows it to skip system headers when traversing the AST.
+- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``. This
+ allows it to avoid matching declarations in system headers .
clang-format
------------
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index e347d0c54d9b0..995d9986b1ba3 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -28,7 +28,6 @@
#include <deque>
#include <memory>
#include <set>
-#include <vector>
namespace clang {
namespace ast_matchers {
@@ -1464,11 +1463,21 @@ bool MatchASTVisitor::objcClassIsDerivedFrom(
return false;
}
+static bool isInSystemHeader(Decl *D) {
+ const SourceManager &SM = D->getASTContext().getSourceManager();
+ const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
+ return SM.isInSystemHeader(Loc);
+}
+
bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
if (!DeclNode) {
return true;
}
+ // Skip declarations in system headers if requested
+ if (Options.SkipSystemHeaders && isInSystemHeader(DeclNode))
+ return true;
+
bool ScopedTraversal =
TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
bool ScopedChildren = TraversingASTChildrenNotSpelledInSource;
@@ -1574,41 +1583,19 @@ bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) {
class MatchASTConsumer : public ASTConsumer {
public:
MatchASTConsumer(MatchFinder *Finder,
- MatchFinder::ParsingDoneTestCallback *ParsingDone,
- const MatchFinderOptions &Options)
- : Finder(Finder), ParsingDone(ParsingDone), Options(Options) {}
+ MatchFinder::ParsingDoneTestCallback *ParsingDone)
+ : Finder(Finder), ParsingDone(ParsingDone) {}
private:
- bool HandleTopLevelDecl(DeclGroupRef DG) override {
- if (Options.SkipSystemHeaders) {
- for (Decl *D : DG) {
- if (!isInSystemHeader(D))
- TraversalScope.push_back(D);
- }
- }
- return true;
- }
-
void HandleTranslationUnit(ASTContext &Context) override {
- if (!TraversalScope.empty())
- Context.setTraversalScope(TraversalScope);
-
if (ParsingDone != nullptr) {
ParsingDone->run();
}
Finder->matchAST(Context);
}
- bool isInSystemHeader(Decl *D) {
- const SourceManager &SM = D->getASTContext().getSourceManager();
- const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
- return SM.isInSystemHeader(Loc);
- }
-
MatchFinder *Finder;
MatchFinder::ParsingDoneTestCallback *ParsingDone;
- const MatchFinderOptions &Options;
- std::vector<Decl *> TraversalScope;
};
} // end namespace
@@ -1727,8 +1714,7 @@ bool MatchFinder::addDynamicMatcher(const internal::DynTypedMatcher &NodeMatch,
}
std::unique_ptr<ASTConsumer> MatchFinder::newASTConsumer() {
- return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone,
- Options);
+ return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone);
}
void MatchFinder::match(const clang::DynTypedNode &Node, ASTContext &Context) {
|
|
@kadircet Are you able to test this PR and confirm it fixes the regressions you encountered? |
|
I think it might be cleaner if reverting the previous solution and introducing this new one would be two separate commits or maybe even separate PRs. |
kadircet
reviewed
Mar 24, 2025
There was a problem hiding this comment.
Are you able to test this PR and confirm it fixes the regressions you encountered?
hi @carlosgalvezp , yes this fixes the particular issues we had, thanks!
Comment on lines
-97
to
-98
| Note: this may lead to false negatives; downstream users may need to adjust | ||
| their checks to preserve existing behavior. |
There was a problem hiding this comment.
i think this still holds. the matching logic is still different than before. despite this version being more conservative, i think there is still value in having an explicit SkipSystemHeaders flag so that affected users can run with old behavior until code fixes are available for affected clang-tidy checks.
There was a problem hiding this comment.
explicit SkipSystemHeaders flag
The problem I have with such a flag is that 1) users will not report issues, just silence them with the flag, therefore 2) people will rely on the flag, so it will need to live forever and 3) we won't be able to unit test all checks with and without the flag.
There was a problem hiding this comment.
- users will not report issues, just silence them with the flag
I agree that this is a risk, but many people use major releases of clang-tidy, hence by the time we get feedback, it'll be impossible for those folks to pick up the fix. I am not sure if it's worth leaving it in broken state for these people until next release. But I am not really a decision maker here, so it's up to you folks.
- we won't be able to unit test all checks with and without the flag
Well, we still have two modes today, it's just controlled by a different flag (warn in system headers on vs off) and we're not testing both. I think this is fine, as we know that SkipSystemHeaders = True is the only risky configuration. It should be fine to keep testing scenario the same, by default test with SkipSystemHeaders = True and only test for SkipSystemHeaders = false when we want to prevent regressions in the future.
I thought about that, yes that might be cleaner. Will do! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…atchFinder
This was introduced here with the purpose of speed up clang-tidy, making it not process system headers:
e4a8969
However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks:
#128150 (comment)
This patch fixes this problem, as follows:
Pros:
Cons:
As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to:
Baseline (without any optimizations). Suppressed 196093 warnings (196093 in non-user code) 10 seconds to run.
Trunk (aggressive optimizations). Suppressed 8050 warnings (8050 in non-user code). 1 second to run.
This patch (conservative optimizations). Suppressed 141779 warnings (141779 in non-user code). 3 seconds to run.
This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy.
Fixes #130618