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