Do not perform the analysis based warning if all warnings are ignored

[ogoffart]

Summary

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.

[lebedev.ri]

You probably want to add a test for this.

[ogoffart]

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".

[lebedev.ri]

In 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.

lib/Sema/AnalysisBasedWarnings.cpp

2084 ↗

(On Diff #123557)

I guess you can try commenting-out that entire check and seeing whether any test fails.

[ogoffart]

lib/Sema/AnalysisBasedWarnings.cpp

2084 ↗

(On Diff #123557)

Commenting out the entire if does not cause any test to fail.

[aaron.ballman]

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.

[ogoffart]

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.

[aaron.ballman]

In 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).

[ogoffart]

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.

[aaron.ballman]

LGTM!

cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp

AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
   // We avoid doing analysis-based warnings when there are errors for
   // two reasons:
   // (1) The CFGs often can't be constructed (if the body is invalid), so
   //     don't bother trying.
   // (2) The code already has problems; running the analysis just takes more
   //     time.
   DiagnosticsEngine &Diags = 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.
   if (cast<DeclContext>(D)->isDependentContext())
     return;
 
   if (Diags.hasUncompilableErrorOccurred()) {
     // Flush out any possibly unreachable diagnostics.

cfe/trunk/unittests/Tooling/ToolingTest.cpp

TEST(ClangToolTest, InjectDiagnosticConsumerInBuildASTs) {
   Tool.setDiagnosticConsumer(&Consumer);
   std::vector<std::unique_ptr<ASTUnit>> ASTs;
   Tool.buildASTs(ASTs);
   EXPECT_EQ(1u, ASTs.size());
   EXPECT_EQ(1u, Consumer.NumDiagnosticsSeen);
 }
 #endif
 
+TEST(runToolOnCode, TestResetDiagnostics) {
+  // This is a tool that resets the diagnostic during the compilation.
+  struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+    std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
+                                                   StringRef) override {
+      struct Consumer : public clang::ASTConsumer {
+        bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+          auto &Diags = (*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<Consumer>();
+    }
+  };
+
+  // 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