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

[Dmitri Gribenko via Phabricator via cfe-commits]

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