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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment 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 "
----------------
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.


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

https://reviews.llvm.org/D66572