[PATCH] D66460: [analyzer] Remove BugReporter.BugTypes.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet.
Herald added a project: clang.

I'm slowly cleaning up `BugReporter` as a preparation for porting clang-tidy to use it exclusively as discussed in http://lists.llvm.org/pipermail/cfe-dev/2019-August/063092.html . Basically, we'll need to make a `BugReport` and a `BugReporter` that both can be compiled with `CLANG_ENABLE_STATIC_ANALYZER=OFF`: no "error nodes", no nothing. So we'll most likely make a clear separation between a basic bug report(er) and a path-sensitive bug report(er), having them inherit from common bug report(er) classes.

While figuring out which field/method goes where, i already made a couple of trivial commits (rC369319 <https://reviews.llvm.org/rC369319>, rC369320 <https://reviews.llvm.org/rC369320>). This one, however, is relatively interesting.

The `BugTypes` field is basically unused, and it definitely doesn't need to be implemented as an //immutable// set (if at all). But once i removed it i started noticing a bunch of null dereferences in `generateDiagnosticForConsumerMap()`:

  3060	  std::unique_ptr<DiagnosticForConsumerMapTy> Out =
  3061	      generatePathDiagnostics(consumers, bugReports);
  3062	 
  3063	  if (Out->empty())   // <== crash: Out is null!
  3064	    return Out;

This was fun to debug because it's obvious by looking at `PathSensitiveBugReporter::generatePathDiagnostics()` that it never returns null:

  2643 std::unique_ptr<DiagnosticForConsumerMapTy>
  2644 PathSensitiveBugReporter::generatePathDiagnostics(...) {
  ...
  2649   auto Out = llvm::make_unique<DiagnosticForConsumerMapTy>();
  ...
  2658   return Out;
  2659 }

The debugger was also sure that we're dealing with a path-sensitive bug reporter here. The answer turned out to be related to this tiny discussion <https://reviews.llvm.org/D63227?id=204353#inline-562789>. Or, rather, here's a report that's worth a thousand words:

F9827911: report-f2e935.html <https://reviews.llvm.org/F9827911>

So i decided to avoid the destructor minefield entirely, even if it means calling flush manually.

Also, ugh, why is CloneChecker subscribed to `check::EndOfTranslationUnit` which is a path-sensitive callback? :/


Repository:
  rC Clang

https://reviews.llvm.org/D66460

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66460.216041.patch
Type: text/x-patch
Size: 4520 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190820/6015b41b/attachment-0001.bin>