[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
This revision was automatically updated to reflect the committed changes.
Closed by commit rL318900: Do not perform the analysis based warning if the
warnings are ignored (authored by ogoffart).
Changed prior to commit:
https://reviews.llvm.org/D40242?vs=123720=124041#toc
Repository:
rL LLVM
https://reviews.llvm.org/D40242
Files:
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/unittests/Tooling/ToolingTest.cpp
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
// time.
DiagnosticsEngine = S.getDiagnostics();
- // Do not do any analysis for declarations in system headers if we are
- // going to just ignore them.
- if (Diags.getSuppressSystemWarnings() &&
- S.SourceMgr.isInSystemHeader(D->getLocation()))
+ // Do not do any analysis if we are going to just ignore them.
+ if (Diags.getIgnoreAllWarnings() ||
+ (Diags.getSuppressSystemWarnings() &&
+ S.SourceMgr.isInSystemHeader(D->getLocation(
return;
// For code in dependent contexts, we'll do this at instantiation time.
Index: cfe/trunk/unittests/Tooling/ToolingTest.cpp
===
--- cfe/trunk/unittests/Tooling/ToolingTest.cpp
+++ cfe/trunk/unittests/Tooling/ToolingTest.cpp
@@ -564,5 +564,31 @@
}
#endif
+TEST(runToolOnCode, TestResetDiagnostics) {
+ // This is a tool that resets the diagnostic during the compilation.
+ struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef) override {
+ struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+ auto = (*D.begin())->getASTContext().getDiagnostics();
+ // Ignore any error
+ Diags.Reset();
+ // Disable warnings because computing the CFG might crash.
+ Diags.setIgnoreAllWarnings(true);
+ return true;
+}
+ };
+ return llvm::make_unique();
+}
+ };
+
+ // Should not crash
+ EXPECT_FALSE(
+ runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
} // end namespace tooling
} // end namespace clang
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
// time.
DiagnosticsEngine = S.getDiagnostics();
- // Do not do any analysis for declarations in system headers if we are
- // going to just ignore them.
- if (Diags.getSuppressSystemWarnings() &&
- S.SourceMgr.isInSystemHeader(D->getLocation()))
+ // Do not do any analysis if we are going to just ignore them.
+ if (Diags.getIgnoreAllWarnings() ||
+ (Diags.getSuppressSystemWarnings() &&
+ S.SourceMgr.isInSystemHeader(D->getLocation(
return;
// For code in dependent contexts, we'll do this at instantiation time.
Index: cfe/trunk/unittests/Tooling/ToolingTest.cpp
===
--- cfe/trunk/unittests/Tooling/ToolingTest.cpp
+++ cfe/trunk/unittests/Tooling/ToolingTest.cpp
@@ -564,5 +564,31 @@
}
#endif
+TEST(runToolOnCode, TestResetDiagnostics) {
+ // This is a tool that resets the diagnostic during the compilation.
+ struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef) override {
+ struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+ auto = (*D.begin())->getASTContext().getDiagnostics();
+ // Ignore any error
+ Diags.Reset();
+ // Disable warnings because computing the CFG might crash.
+ Diags.setIgnoreAllWarnings(true);
+ return true;
+}
+ };
+ return llvm::make_unique();
+}
+ };
+
+ // Should not crash
+ EXPECT_FALSE(
+ runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
} // end namespace tooling
} // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
ogoffart updated this revision to Diff 123720.
ogoffart added a comment.
Herald added a subscriber: klimek.
I added a test.
I did not add such test before because i know that calling Reset() on the
diagnostics is kind of a hack.
This change does not have any behavioral difference normally, it is just an
optimization that skips the computation of the CFG if all warnings are ignored.
https://reviews.llvm.org/D40242
Files:
lib/Sema/AnalysisBasedWarnings.cpp
unittests/Tooling/ToolingTest.cpp
Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -533,5 +533,31 @@
}
#endif
+TEST(runToolOnCode, TestResetDiagnostics) {
+ // This is a tool that resets the diagnostic during the compilation.
+ struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef) override {
+ struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+ auto = (*D.begin())->getASTContext().getDiagnostics();
+ // Ignore any error
+ Diags.Reset();
+ // Disable warnings because computing the CFG might crash.
+ Diags.setIgnoreAllWarnings(true);
+ return true;
+}
+ };
+ return llvm::make_unique();
+}
+ };
+
+ // Should not crash
+ EXPECT_FALSE(
+ runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
} // end namespace tooling
} // end namespace clang
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
// time.
DiagnosticsEngine = S.getDiagnostics();
- // Do not do any analysis for declarations in system headers if we are
- // going to just ignore them.
- if (Diags.getSuppressSystemWarnings() &&
- S.SourceMgr.isInSystemHeader(D->getLocation()))
+ // Do not do any analysis if we are going to just ignore them.
+ if (Diags.getIgnoreAllWarnings() ||
+ (Diags.getSuppressSystemWarnings() &&
+ S.SourceMgr.isInSystemHeader(D->getLocation(
return;
// For code in dependent contexts, we'll do this at instantiation time.
Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -533,5 +533,31 @@
}
#endif
+TEST(runToolOnCode, TestResetDiagnostics) {
+ // This is a tool that resets the diagnostic during the compilation.
+ struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef) override {
+ struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+ auto = (*D.begin())->getASTContext().getDiagnostics();
+ // Ignore any error
+ Diags.Reset();
+ // Disable warnings because computing the CFG might crash.
+ Diags.setIgnoreAllWarnings(true);
+ return true;
+}
+ };
+ return llvm::make_unique();
+}
+ };
+
+ // Should not crash
+ EXPECT_FALSE(
+ runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
} // end namespace tooling
} // end namespace clang
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
// time.
DiagnosticsEngine = S.getDiagnostics();
- // Do not do any analysis for declarations in system headers if we are
- // going to just ignore them.
- if (Diags.getSuppressSystemWarnings() &&
- S.SourceMgr.isInSystemHeader(D->getLocation()))
+ // Do not do any analysis if we are going to just ignore them.
+ if (Diags.getIgnoreAllWarnings() ||
+ (Diags.getSuppressSystemWarnings() &&
+ S.SourceMgr.isInSystemHeader(D->getLocation(
return;
// For code in dependent contexts, we'll do this at instantiation time.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
aaron.ballman added a comment. In https://reviews.llvm.org/D40242#931173, @ogoffart wrote: > > You should add a test case that demonstrates code which would otherwise > > trigger an analysis-based warning but doesn't due to disabling all warnings. > > No warnings are triggered. Because the diagnostic engine ignores them. > This just optimize the code so no analysis are preformed when no warning can > be triggered. Correct -- I'm asking for a test that ensures we don't regress the intended behavior of this patch by accident at some point (behavioral changes should always have at least one accompanying test). https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
ogoffart marked an inline comment as done. ogoffart added a comment. > You should add a test case that demonstrates code which would otherwise > trigger an analysis-based warning but doesn't due to disabling all warnings. No warnings are triggered. Because the diagnostic engine ignores them. This just optimize the code so no analysis are preformed when no warning can be triggered. https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
aaron.ballman added a comment. You should add a test case that demonstrates code which would otherwise trigger an analysis-based warning but doesn't due to disabling all warnings. https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
ogoffart marked an inline comment as done. ogoffart added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:2084 + // Do not do any analysis if we are going to just ignore them. + if (Diags.getIgnoreAllWarnings() || + (Diags.getSuppressSystemWarnings() && lebedev.ri wrote: > I guess you can try commenting-out that entire check and seeing whether any > test fails. Commenting out the entire if does not cause any test to fail. https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
lebedev.ri added a comment. In https://reviews.llvm.org/D40242#930125, @ogoffart wrote: > I do not know how to add a test: there is no real visible change for clang. > It just should be faster when passing "-w". Oh right, i was thinking of something else there. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:2084 + // Do not do any analysis if we are going to just ignore them. + if (Diags.getIgnoreAllWarnings() || + (Diags.getSuppressSystemWarnings() && I guess you can try commenting-out that entire check and seeing whether any test fails. https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
ogoffart added a comment. I do not know how to add a test: there is no real visible change for clang. It just should be faster when passing "-w". https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
lebedev.ri added a comment. You probably want to add a test for this. https://reviews.llvm.org/D40242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored
ogoffart created this revision. Do not perform the analysis based warning if all warnings are ignored. This saves some cycles when compiling with "-w". But more importantly, for my tool, this fixes a potential crash which may happen on invalid code. Because the analysis might compute the CFG which crashes if the code contains invalid declaration. This does not happen normally with because we also don't perform these analysis if there was an error. But the tool is better at recovering and hit this condition. https://reviews.llvm.org/D40242 Files: lib/Sema/AnalysisBasedWarnings.cpp Index: lib/Sema/AnalysisBasedWarnings.cpp === --- lib/Sema/AnalysisBasedWarnings.cpp +++ lib/Sema/AnalysisBasedWarnings.cpp @@ -2080,10 +2080,10 @@ // time. DiagnosticsEngine = S.getDiagnostics(); - // Do not do any analysis for declarations in system headers if we are - // going to just ignore them. - if (Diags.getSuppressSystemWarnings() && - S.SourceMgr.isInSystemHeader(D->getLocation())) + // Do not do any analysis if we are going to just ignore them. + if (Diags.getIgnoreAllWarnings() || + (Diags.getSuppressSystemWarnings() && + S.SourceMgr.isInSystemHeader(D->getLocation( return; // For code in dependent contexts, we'll do this at instantiation time. Index: lib/Sema/AnalysisBasedWarnings.cpp === --- lib/Sema/AnalysisBasedWarnings.cpp +++ lib/Sema/AnalysisBasedWarnings.cpp @@ -2080,10 +2080,10 @@ // time. DiagnosticsEngine = S.getDiagnostics(); - // Do not do any analysis for declarations in system headers if we are - // going to just ignore them. - if (Diags.getSuppressSystemWarnings() && - S.SourceMgr.isInSystemHeader(D->getLocation())) + // Do not do any analysis if we are going to just ignore them. + if (Diags.getIgnoreAllWarnings() || + (Diags.getSuppressSystemWarnings() && + S.SourceMgr.isInSystemHeader(D->getLocation( return; // For code in dependent contexts, we'll do this at instantiation time. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
