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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
