[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:600
+public:
+  typedef std::function<std::string(BugReporterContext &, BugReport &)>
+      Callback;
----------------
Szelethus wrote:
> Prefer using.
Thx!~


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:348
+/// additional PathDiagnosticEventPieces along the path.
+class NoteTag : public ProgramPointTag {
+public:
----------------
baloghadamsoftware wrote:
> I am not sure whether `BugReporterVisitors.h` is the best place for this structure. I would rather put this into `ProgramPoint.h` or maybe `BugReporter.h`.
Moved to `BugReporter.h`.

`ProgramPoint.h` is too low-level, it's not even in `libStaticAnalyzer*`, so talking about stuff like `BugReporterContext` within it sounds a bit hard to get compiled.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:386-398
+  // Manage memory for NoteTag objects.
+  class Factory {
+    std::vector<std::unique_ptr<NoteTag>> Tags;
+
+  public:
+    const NoteTag *makeNoteTag(Callback &&Cb) {
+      // We cannot use make_unique because we cannot access the private
----------------
Szelethus wrote:
> Hmmm, did you consider the other memory management options we have in LLVM, like `BumpPtrAllocator`? Sounds a bit better for this use case.
Hmm, apparently i didn't. Fxd.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:400
+
+  friend class Factory;
+  friend class TagVisitor;
----------------
xazax.hun wrote:
> Isn't this redundant?
It isn't - i made a private constructor as usual.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58367/new/

https://reviews.llvm.org/D58367