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