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

Tue Mar 12 05:32:52 PDT 2019

Szelethus added a comment.

Would `NoteTag`s be displayed in a dumped exploded graph?

In D58367#1405185 <https://reviews.llvm.org/D58367#1405185>, @NoQ wrote:

> In D58367#1404722 <https://reviews.llvm.org/D58367#1404722>, @Charusso wrote:
>
> > So with that, I would out-chain this patch from the MIG-patches because it is a lot more serious problem-set. I also would like to ask you to take it more serious, as all your mentioned benefits will rely on this simple and cool approach, so it has to be flexible as possible.
>
>
> I've been thinking for about a month about this until now, and i'm actually very surprised that i see no obvious flexibility issues here. Stateful visitors (eg., the ones that only highlight the *last* interesting event) can be easily implemented by keeping via lambdas as a private state data in the bug reporter. Mutually recursive systems of multiple visitors that add each other dynamically during the visit (such as the `trackExpressionValue` that's infamous for that exact reason) should be (and most likely could be) untangled anyway, and once they're untangled (eg., by keeping just one instance of the visitor while dynamically updating its tracking information), the flexibility issue disappears; i'm almost happy that it would no longer be possible to entangle the code that way. Dunno, this is weird - usually i quickly come up with examples of why something wouldn't work and decide to implement it anyway, but this time i'm surprisingly secure about implementing it.

In D58367#1402847 <https://reviews.llvm.org/D58367#1402847>, @NoQ wrote:

> In D58367#1402796 <https://reviews.llvm.org/D58367#1402796>, @Szelethus wrote:
>
> > 2. I guess most of the visitors could be changed to this format, do you have plans to convert them? I'll happily land a hand, because it sounds like a big chore. I guess that would also test this implementation fairly well.
>
>
> I don't have an immediate plan but i'll definitely convert visitors when i touch them and get annoyed. Also i believe that this new functionality is much more useful for //core// visitors than for checker visitors, simply because there's much more information to reverse-engineer in every visitor.

As I understand it, this solution could be used to entirely get rid of the current bugreporter visitor structure (at least for checkers), right? The discussion seems to conclude that this is just as general, far easier to understand, far easier to implement, and is basically better in every regard without a hit to performance? Because if so, I'm definitely against supporting two concurrent implementations of the same functionality -- in fact, we should even just forbid checkers to add custom visitors.

I can't say much about the rest of the discussion, seems like you two are far more knowledgable on this topic than me :^)

> it hurts me when i see all of them duplicated in the visitor as a military-grade portion of spaghetti.

Made my day :D

================
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
----------------
Hmmm, did you consider the other memory management options we have in LLVM, like `BumpPtrAllocator`? Sounds a bit better for this use case.

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

https://reviews.llvm.org/D58367