gribozavr added a comment.
Sorry for the silly comments, but my main point is, I guess, that I don't quite
understand the design towards which you are refactoring, and to meaningfully
review patches I need to be on the same page.
================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:75
/// individual bug reports.
class BugReport : public llvm::ilist_node<BugReport> {
public:
----------------
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.
================
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;
----------------
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?
================
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.
----------------
I don't think `getUniqueingDecl()` and `getDeclWithIssue()` make sense for most
ClangTidy checkers.
================
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) {
----------------
We don't care about MSVC 2013 now.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:174
+ /// This location is used by clients rendering diagnostics.
+ virtual PathDiagnosticLocation getLocation(const SourceManager &SM) const {
+ assert(Location.isValid());
----------------
Another location-related method... I guess I would appreciate a more abstract
writeup about what BugReport's data model is (in its doc comment).
================
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 "
----------------
Ranges should be associated with a message.
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
