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

[Artem Dergachev via Phabricator via cfe-commits]

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


================
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 "
----------------
NoQ wrote:
> gribozavr wrote:
> > NoQ wrote:
> > > 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()`.
> > I see. What looks weird to me is that methods related to the warning itself are on `BugReport`, but notes and fixits are their own data structures. It creates an inconsistent API, and makes notes and fixits feel bolted on.
> > 
> > Do you think it would make sense to change the API to be more uniform?
> Hmm, i guess this is an artifact of how path-sensitive checkers usually emits warnings and their respective notes in completely different parts of their code (warnings come from the checker itself, path notes are generated by so-called "bug visitors" which aren't necessarily even a part of the checker).
> 
> Generally we need our notes to be attached to their respective warnings; say, in HTML report they need to be displayed on the same HTML page. But yeah, we should make our APIs more uniform because there's an obvious duplication of effort.
> 
> I also suspect that we'll need a new API in general, because in the current shape the `BugReporter` will look fairly alien and overly-complicated to clang-tidy developers that are used to the conciseness of `diag() << ...`. I'm not sure if it'll boil down to providing convenient wrappers or i'll prefer to rewrite our checkers as well. I think we should talk about this separately on the mailing list.
http://lists.llvm.org/pipermail/cfe-dev/2019-September/063229.html

Basically, `PathDiagnostic` is the uniform API that we've been looking for. It's a vector of `PathDiagnosticPiece`s each of which represents a note of certain kind (path note, normal note, control flow note, etc.). `BugReporter` is a mechanism for converting a `BugReport` into a `PathDiagnostic`. For path-sensitive reports this mechanism is extremely sophisticated: the checker only supplies a single `ExplodedNode` that corresponds to the end of path and the `BugReporter` automatically adds notes (often dozens of them, sometimes hundreds) to explain the path. For path-insensitive reports the conversion is extremely trivial and therefore there's very little motivation to use the `BugReporter` when only path-insensitive reports are expected.


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

https://reviews.llvm.org/D66572