Page MenuHomePhabricator

[analyzer] Decouple NoteTag from its Factory
ClosedPublic

Authored by vsavchenko on Fri, Jun 11, 11:04 AM.

Details

Summary

This allows us to create other types of tags that carry useful
bits of information alongside.

Diff Detail

Event Timeline

vsavchenko created this revision.Fri, Jun 11, 11:04 AM
vsavchenko requested review of this revision.Fri, Jun 11, 11:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jun 11, 11:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ added inline comments.Fri, Jun 11, 11:52 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
777

It sounds like NoteTag isn't<DataTag> despite inheriting from it.

NoQ accepted this revision.Fri, Jun 11, 12:09 PM

P.S. I like this design!

I'm trying to remember why we needed a factory in the first place. I think a lot of tags work just fine as static variables. In case of D104136 IIUC IdentityCall could always be discovered as the current program point and its Source is 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.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
777

Wait nvm, DataTag doesn't provide a classof at all. This means we can't write isa<DataTag> at all, right?

This revision is now accepted and ready to land.Fri, Jun 11, 12:09 PM

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 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.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
777

Yes, it's purely abstract. ProgramPointTag also doesn't have classof

NoQ added inline comments.Mon, Jun 14, 8:47 PM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
777

Something being purely abstract isn't a problem on its own; there are a lot of abstract classes that implement classof(), such as Expr. It's more of an artifact of the current implementation strategy.

Ok np though, this doesn't seem to be a real problem.

This revision was landed with ongoing or failed builds.Tue, Jun 15, 1:59 AM
This revision was automatically updated to reflect the committed changes.
vsavchenko added inline comments.Tue, Jun 15, 2:08 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
777

That's true. And considering that we want different checkers to implement this on their own, we probably don't want to have one central enum listing all possible subtypes so that we can isa non-leaf types.
But unlike Expr, DataTag doesn't provide any information, so it's useless for people to check if something is a data tag or cast to it.