[PATCH] D104135: [analyzer] Decouple NoteTag from its Factory

Fri Jun 11 12:55:33 PDT 2021

vsavchenko added a comment.

In D104135#2814081 <https://reviews.llvm.org/D104135#2814081>, @NoQ wrote:

> P.S. I like this design!
>
> I'm trying to remember why we needed a factory in the first place.

Who's gonna carry your callbacks for ya, huh?

> I think a lot of tags work just fine as static variables.

Sure, but if you have data, let's say it's something that doesn't need to be stored somewhere to prolong its lifetime, you still need to have static variable for each possible value of that piece of data.  Not really feasible in the vast majority of cases.  Because you might need to refer to the same tag with different possible values.

> In case of D104136 <https://reviews.llvm.org/D104136> IIUC `IdentityCall` could always be discovered as the current program point...

That's true, I put this one as "just in case".

> ...and its `Source` is its only argument...

Not really, there is also case when the call is `IdentityThis`, so its `Source` is not its only argument.

> ...so all we need is a marker that says "we did indeed model this code as identity" so it could potentially be allocated statically. But when the tag really needs to carry more data then there's probably no way around the factory.

================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:777
   static bool classof(const ProgramPointTag *T) {
     return T->getTagKind() == &Kind;
   }
----------------
NoQ wrote:
> NoQ wrote:
> > It sounds like `NoteTag` `isn't<DataTag>` despite inheriting from it.
> Wait nvm, `DataTag` doesn't provide a `classof` at all. This means we can't write `isa<DataTag>` at all, right?
Yes, it's purely abstract.  `ProgramPointTag` also doesn't have `classof`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104135