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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:777
   static bool classof(const ProgramPointTag *T) {
 return T->getTagKind() == 
   }

NoQ wrote:
> vsavchenko wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > It sounds like `NoteTag` `isn't` despite inheriting from it.
> > > Wait nvm, `DataTag` doesn't provide a `classof` at all. This means we 
> > > can't write `isa` at all, right?
> > Yes, it's purely abstract.  `ProgramPointTag` also doesn't have `classof`
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGeadd54f2741f: [analyzer] Decouple NoteTag from its Factory 
(authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104135

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -219,13 +219,14 @@
   // and we're taking the path that skips virtual base constructors.
   if (L.getSrc()->getTerminator().isVirtualBaseBranch() &&
   L.getDst() == *L.getSrc()->succ_begin()) {
-ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+ProgramPoint P = L.withTag(getDataTags().make(
 [](BugReporterContext &, PathSensitiveBugReport &) -> std::string {
   // TODO: Just call out the name of the most derived class
   // when we know it.
   return "Virtual base initialization skipped because "
  "it has already been handled by the most derived class";
-}, /*IsPrunable=*/true));
+},
+/*IsPrunable=*/true));
 // Perform the transition.
 ExplodedNodeSet Dst;
 NodeBuilder Bldr(Pred, Dst, BuilderCtx);
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -428,8 +428,7 @@
   SymbolManager () { return SymMgr; }
   MemRegionManager () { return MRMgr; }
 
-  NoteTag::Factory () { return Engine.getNoteTags(); }
-
+  DataTag::Factory () { return Engine.getDataTags(); }
 
   // Functions for external checking of whether we have unfinished work
   bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); }
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -96,9 +96,10 @@
   /// (This data is owned by AnalysisConsumer.)
   FunctionSummariesTy *FunctionSummaries;
 
-  /// Add path note tags along the path when we see that something interesting
-  /// is happening. This field is the allocator for such tags.
-  NoteTag::Factory NoteTags;
+  /// Add path tags with some useful data along the path when we see that
+  /// something interesting is happening. This field is the allocator for such
+  /// tags.
+  DataTag::Factory DataTags;
 
   void generateNode(const ProgramPoint ,
 ProgramStateRef State,
@@ -200,7 +201,7 @@
   /// Enqueue a single node created as a result of statement processing.
   void enqueueStmtNode(ExplodedNode *N, const CFGBlock *Block, unsigned Idx);
 
-  NoteTag::Factory () { return NoteTags; }
+  DataTag::Factory () { return DataTags; }
 };
 
 // TODO: Turn into a class.
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -255,7 +255,7 @@
   ///to omit the note from the report if it would make the displayed
   ///bug path significantly shorter.
   const NoteTag *getNoteTag(NoteTag::Callback &, bool IsPrunable = false) {
-return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable);
+return Eng.getDataTags().make(std::move(Cb), IsPrunable);
   }
 
   /// A shorthand version of getNoteTag that doesn't require you to accept
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -725,14 +725,43 @@
   }
 };
 
+/// The tag that carries some information with it.
+///
+/// It can be valuable to produce tags with some bits of information and later
+/// reuse them for a better diagnostic.
+///
+/// Please make sure that derived class' constuctor is private and that the user
+/// can only create objects using DataTag::Factory.  This also means that
+/// DataTag::Factory should be friend for every derived class.

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:777
   static bool classof(const ProgramPointTag *T) {
 return T->getTagKind() == 
   }

vsavchenko wrote:
> NoQ wrote:
> > NoQ wrote:
> > > It sounds like `NoteTag` `isn't` despite inheriting from it.
> > Wait nvm, `DataTag` doesn't provide a `classof` at all. This means we can't 
> > write `isa` at all, right?
> Yes, it's purely abstract.  `ProgramPointTag` also doesn't have `classof`
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In 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  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() == 
   }

NoQ wrote:
> NoQ wrote:
> > It sounds like `NoteTag` `isn't` despite inheriting from it.
> Wait nvm, `DataTag` doesn't provide a `classof` at all. This means we can't 
> write `isa` 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

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.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:777
   static bool classof(const ProgramPointTag *T) {
 return T->getTagKind() == 
   }

NoQ wrote:
> It sounds like `NoteTag` `isn't` despite inheriting from it.
Wait nvm, `DataTag` doesn't provide a `classof` at all. This means we can't 
write `isa` at all, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:777
   static bool classof(const ProgramPointTag *T) {
 return T->getTagKind() == 
   }

It sounds like `NoteTag` `isn't` despite inheriting from it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko created this revision.
vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus, 
manas, RedDocMD.
Herald added subscribers: ASDenysPetrov, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104135

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -219,13 +219,14 @@
   // and we're taking the path that skips virtual base constructors.
   if (L.getSrc()->getTerminator().isVirtualBaseBranch() &&
   L.getDst() == *L.getSrc()->succ_begin()) {
-ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+ProgramPoint P = L.withTag(getDataTags().make(
 [](BugReporterContext &, PathSensitiveBugReport &) -> std::string {
   // TODO: Just call out the name of the most derived class
   // when we know it.
   return "Virtual base initialization skipped because "
  "it has already been handled by the most derived class";
-}, /*IsPrunable=*/true));
+},
+/*IsPrunable=*/true));
 // Perform the transition.
 ExplodedNodeSet Dst;
 NodeBuilder Bldr(Pred, Dst, BuilderCtx);
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -428,8 +428,7 @@
   SymbolManager () { return SymMgr; }
   MemRegionManager () { return MRMgr; }
 
-  NoteTag::Factory () { return Engine.getNoteTags(); }
-
+  DataTag::Factory () { return Engine.getDataTags(); }
 
   // Functions for external checking of whether we have unfinished work
   bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); }
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -96,9 +96,10 @@
   /// (This data is owned by AnalysisConsumer.)
   FunctionSummariesTy *FunctionSummaries;
 
-  /// Add path note tags along the path when we see that something interesting
-  /// is happening. This field is the allocator for such tags.
-  NoteTag::Factory NoteTags;
+  /// Add path tags with some useful data along the path when we see that
+  /// something interesting is happening. This field is the allocator for such
+  /// tags.
+  DataTag::Factory DataTags;
 
   void generateNode(const ProgramPoint ,
 ProgramStateRef State,
@@ -200,7 +201,7 @@
   /// Enqueue a single node created as a result of statement processing.
   void enqueueStmtNode(ExplodedNode *N, const CFGBlock *Block, unsigned Idx);
 
-  NoteTag::Factory () { return NoteTags; }
+  DataTag::Factory () { return DataTags; }
 };
 
 // TODO: Turn into a class.
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -255,7 +255,7 @@
   ///to omit the note from the report if it would make the displayed
   ///bug path significantly shorter.
   const NoteTag *getNoteTag(NoteTag::Callback &, bool IsPrunable = false) {
-return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable);
+return Eng.getDataTags().make(std::move(Cb), IsPrunable);
   }
 
   /// A shorthand version of getNoteTag that doesn't require you to accept
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -725,14 +725,43 @@
   }
 };
 
+/// The tag that carries some information with it.
+///
+/// It can be valuable to produce tags with some bits of information and later
+/// reuse them for a better diagnostic.
+///
+/// Please make sure that derived class' constuctor is