[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 11 inline comments as done.
NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:75
 /// individual bug reports.
 class BugReport : public llvm::ilist_node<BugReport> {
 public:
----------------
gribozavr wrote:
> Szelethus wrote:
> > Shouldn't we make this an abstract class?
> I'm not sure that intrusive linked list is the right data structure for the job. I'd personally put bug reports into a vector and make a custom data structure if a vector becomes a performance problem.
> Shouldn't we make this an abstract class?

Mmm, yeah, moved virtual methods from `BugReport` to `BasicBugReport` whenever `PathSensitiveBugReport` immediately overrides them and it looks much better now!

> I'm not sure that intrusive linked list is the right data structure for the job.

Me neither, but it seems to be orthogonal to this patch, and this patch is already huge, so i'll do this in a follow-up patch, ok?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:122
+  /// Get the location on which the report should be uniqued.
+  virtual PathDiagnosticLocation getUniqueingLocation() const {
+    return Location;
----------------
gribozavr wrote:
> Where can I read about the uniqueing logic? Does it mean that out of two bug reports with the same location only one gets displayed, regardless of other properties?
Added some comments ^^


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:126-135
+  /// Get the declaration containing the uniqueing location.
+  virtual const Decl *getUniqueingDecl() const {
+    return DeclWithIssue;
+  }
+
+  /// Return the canonical declaration, be it a method or class, where
+  /// this issue semantically occurred.
----------------
gribozavr wrote:
> I don't think `getUniqueingDecl()` and `getDeclWithIssue()` make sense for most ClangTidy checkers.
`getDeclWithIssue()` corresponds to the "Function/Method" column we have in our scan-build index page:

{F9881207}

This column is not super important but probably kinda nice to have. I don't mind leaving it empty for clang-tidy checks, or possibly auto-detect it post factum when possible (e.g., find the smallest decl in which the warning location is contained).

`getUniqueingDecl()` is, as far as i understand, only currently used by the issue hash mechanism which governs deduplication across translation units (e.g., we emit a warning against the same header in multiple translation units - it's vital for the static analyzer because it's not uncommon for an execution path that starts in the main file to end in a header). And even then, it's only present in plist output. I'm not convinced it's useful at all. It's likely that we can remove it.

Also clang-tidy checks wouldn't need to specify their `UniqueingDecl` manually as it silently defaults to their `DeclWithIssue`.

So basically we have these two methods on the base class for bureaucratic reasons: our existing algorithms handle all kinds of warnings uniformly based on these computed properties of bug reports. I think we should be perfectly fine if some kinds of bug reports return null from them, indicating that "this property doesn't make sense for them". We could instead reinvent protocols on top of our custom RTTI ("does this warning conform to `Uniqueable`? if so, i'll try to unique it"), but i don't think it's worth the complexity.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:159
+  // FIXME: Instead of making an overload, we could have default-initialized
+  // Ranges with {}, however it crashes the MSVC 2013 compiler.
+  void addNote(StringRef Msg, const PathDiagnosticLocation &Pos) {
----------------
gribozavr wrote:
> We don't care about MSVC 2013 now.
Woohoo!


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:186
+  /// ranges.
+  void addRange(SourceRange R) {
+    assert((R.isValid() || Ranges.empty()) && "Invalid range can only be used "
----------------
gribozavr wrote:
> Ranges should be associated with a message.
Mmm, what do you mean?

Currently these ranges are attached to the warning message, path note messages can't have ranges, and extra path-insensitive notes can have a separate set of ranges attached to them by passing them through `addNote()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66572/new/

https://reviews.llvm.org/D66572