[PATCH] D151373: [libclang] Expose arguments of clang::annotate{_type}

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D151373#4533550 , @fridtjof wrote:

> I fixed the failing IBOutletCollection.m test - there was code to do special 
> handling for the respective attribute type, which my patch accidentally 
> skipped previously. I reordered the generic attribute handling to come after 
> the special cases in VisitChildren.

Nice!

> I cleaned up my unit test. After trying a bit to fix it for testing 
> annotate_type as well, I found out that for some unknown reason it is not 
> exposed like usual attributes (maybe because it's a type attribute?).
> Therefore, I decided to drop it from the test altogether.

I think we should drop the `annotate_type` functionality from the patch 
entirely rather than add it without any test coverage knowing it doesn't seem 
to work as expected.


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

https://reviews.llvm.org/D151373

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

[PATCH] D151373: [libclang] Expose arguments of clang::annotate{_type}

[Fridtjof Mund via Phabricator via cfe-commits]

fridtjof updated this revision to Diff 544142.
fridtjof added a comment.

I fixed the failing IBOutletCollection.m test - there was code to do special 
handling for the respective attribute type, which my patch accidentally skipped 
previously. I reordered the generic attribute handling to come after the 
special cases in VisitChildren.

I cleaned up my unit test. After trying a bit to fix it for testing 
annotate_type as well, I found out that for some unknown reason it is not 
exposed like usual attributes (maybe because it's a type attribute?).
Therefore, I decided to drop it from the test altogether.


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

https://reviews.llvm.org/D151373

Files:
  clang/docs/ReleaseNotes.rst
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CursorVisitor.h
  clang/unittests/libclang/LibclangTest.cpp

Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -1246,6 +1246,50 @@
   EXPECT_EQ(fromCXString(clang_getCursorSpelling(*staticAssertCsr)), "");
 }
 
+TEST_F(LibclangParseTest, ExposesAnnotateArgs) {
+  const char testSource[] = R"cpp(
+[[clang::annotate("category", 42)]]
+void func() {}
+)cpp";
+  std::string fileName = "main.cpp";
+  WriteFile(fileName, testSource);
+
+  const char *Args[] = {"-xc++"};
+  ClangTU = clang_parseTranslationUnit(Index, fileName.c_str(), Args, 1,
+   nullptr, 0, TUFlags);
+
+  int attrCount = 0;
+
+  Traverse(
+  [](CXCursor cursor, CXCursor parent) -> CXChildVisitResult {
+if (cursor.kind == CXCursor_AnnotateAttr) {
+  int childCount = 0;
+  clang_visitChildren(
+  cursor,
+  [](CXCursor child, CXCursor,
+ CXClientData data) -> CXChildVisitResult {
+int *pcount = static_cast(data);
+
+// we only expect one argument here, so bail otherwise
+EXPECT_EQ(*pcount, 0);
+
+auto *result = clang_Cursor_Evaluate(child);
+EXPECT_NE(result, nullptr);
+EXPECT_EQ(clang_EvalResult_getAsInt(result), 42);
+++*pcount;
+
+return CXChildVisit_Recurse;
+  },
+  );
+  attrCount++;
+  return CXChildVisit_Continue;
+}
+return CXChildVisit_Recurse;
+  });
+
+  EXPECT_EQ(attrCount, 1);
+}
+
 class LibclangRewriteTest : public LibclangParseTest {
 public:
   CXRewriter Rew = nullptr;
Index: clang/tools/libclang/CursorVisitor.h
===
--- clang/tools/libclang/CursorVisitor.h
+++ clang/tools/libclang/CursorVisitor.h
@@ -276,7 +276,9 @@
   bool IsInRegionOfInterest(CXCursor C);
   bool RunVisitorWorkList(VisitorWorkList );
   void EnqueueWorkList(VisitorWorkList , const Stmt *S);
+  void EnqueueWorkList(VisitorWorkList , const Attr *A);
   LLVM_ATTRIBUTE_NOINLINE bool Visit(const Stmt *S);
+  LLVM_ATTRIBUTE_NOINLINE bool Visit(const Attr *A);
 
 private:
   std::optional handleDeclForVisitation(const Decl *D);
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -23,6 +23,7 @@
 #include "CursorVisitor.h"
 #include "clang-c/FatalErrorHandler.h"
 #include "clang/AST/Attr.h"
+#include "clang/AST/AttrVisitor.h"
 #include "clang/AST/DeclObjCCommon.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
@@ -575,6 +576,13 @@
   A->getInterfaceLoc()->getTypeLoc().getBeginLoc(), TU));
   }
 
+  if (clang_isAttribute(Cursor.kind)) {
+if (const Attr *A = getCursorAttr(Cursor))
+  return Visit(A);
+
+return false;
+  }
+
   // If pointing inside a macro definition, check if the token is an identifier
   // that was ever defined as a macro. In such a case, create a "pseudo" macro
   // expansion cursor for that token.
@@ -2089,7 +2097,8 @@
 (SourceLocation::UIntTy)(uintptr_t)data[1]);
   }
 };
-class EnqueueVisitor : public ConstStmtVisitor {
+class EnqueueVisitor : public ConstStmtVisitor,
+   public ConstAttrVisitor {
   friend class OMPClauseEnqueue;
   VisitorWorkList 
   CXCursor Parent;
@@ -2231,6 +2240,10 @@
   void VisitOMPTargetTeamsDistributeSimdDirective(
   const OMPTargetTeamsDistributeSimdDirective *D);
 
+  // Attributes
+  void VisitAnnotateAttr(const AnnotateAttr *A);
+  void VisitAnnotateTypeAttr(const AnnotateTypeAttr *A);
+
 private:
   void AddDeclarationNameInfo(const Stmt *S);
   void AddNestedNameSpecifierLoc(NestedNameSpecifierLoc Qualifier);
@@ -2242,6 +2255,11 @@
   void AddTypeLoc(TypeSourceInfo *TI);
   void EnqueueChildren(const Stmt *S);
   void EnqueueChildren(const OMPClause *S);
+  template ::value ||
+ 

[PATCH] D151373: [libclang] Expose arguments of clang::annotate{_type}

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think precommit CI may have found a relevant failure:

   TEST 'Clang :: Index/IBOutletCollection.m' FAILED 

  Script:
  --
  : 'RUN: at line 8';   
c:\ws\w5\llvm-project\premerge-checks\build\bin\c-index-test.exe 
-cursor-at=C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m:4:24
 C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m | 
c:\ws\w5\llvm-project\premerge-checks\build\bin\filecheck.exe 
-check-prefix=CHECK-CURSOR 
C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m
  : 'RUN: at line 11';   
c:\ws\w5\llvm-project\premerge-checks\build\bin\c-index-test.exe 
-test-annotate-tokens=C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m:4:1:5:1
 C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m | 
c:\ws\w5\llvm-project\premerge-checks\build\bin\filecheck.exe 
-check-prefix=CHECK-TOK 
C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m
  --
  Exit Code: 1
   
  Command Output (stdout):
  --
  $ ":" "RUN: at line 8"
  $ "c:\ws\w5\llvm-project\premerge-checks\build\bin\c-index-test.exe" 
"-cursor-at=C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m:4:24"
 "C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m"
  $ "c:\ws\w5\llvm-project\premerge-checks\build\bin\filecheck.exe" 
"-check-prefix=CHECK-CURSOR" 
"C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m"
  # command stderr:
  
C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m:9:18:
 error: CHECK-CURSOR: expected string not found in input
  // CHECK-CURSOR: ObjCClassRef=Test:3:12
   ^
  :1:1: note: scanning from here
  4:3 attribute(iboutletcollection)= [IBOutletCollection=ObjCInterface] 
Extent=[4:3 - 4:27]
  ^
  :1:54: note: possible intended match here
  4:3 attribute(iboutletcollection)= [IBOutletCollection=ObjCInterface] 
Extent=[4:3 - 4:27]
   ^
   
  Input file: 
  Check file: 
C:\ws\w5\llvm-project\premerge-checks\clang\test\Index\IBOutletCollection.m
   
  -dump-input=help explains the following input dump.
   
  Input was:
  <<
 1: 4:3 attribute(iboutletcollection)= 
[IBOutletCollection=ObjCInterface] Extent=[4:3 - 4:27]
  check:9'0 
X~
 error: no match found
  check:9'1  ?  
   possible intended match
  >>
   
  error: command failed with exit status: 1
   
  --
   
  




Comment at: clang/unittests/libclang/LibclangTest.cpp:1176
+TEST_F(LibclangParseTest, ExposedAnnotateArgs) {
+  // TODO
+  const char testSource[] = R"cpp(

Spurious comment?



Comment at: clang/unittests/libclang/LibclangTest.cpp:1178
+  const char testSource[] = R"cpp(
+int [[clang::annotate_type("category_type", 42)]] f;
+

We don't seem to be testing this?


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

https://reviews.llvm.org/D151373

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

[PATCH] D151373: [libclang] Expose arguments of clang::annotate{_type}

[Fridtjof Mund via Phabricator via cfe-commits]

fridtjof updated this revision to Diff 530219.
fridtjof added a comment.

- Deduplicated code by using templates (looks a bit ridicilous though)
- Release notes entry added
- Unit test added


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

https://reviews.llvm.org/D151373

Files:
  clang/docs/ReleaseNotes.rst
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CursorVisitor.h
  clang/unittests/libclang/LibclangTest.cpp

Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -1172,6 +1172,48 @@
   });
 }
 
+TEST_F(LibclangParseTest, ExposedAnnotateArgs) {
+  // TODO
+  const char testSource[] = R"cpp(
+int [[clang::annotate_type("category_type", 42)]] f;
+
+[[clang::annotate("category", 42)]]
+void func() {}
+)cpp";
+  std::string fileName = "main.cpp";
+  WriteFile(fileName, testSource);
+
+  const char *Args[] = {"-xc++"};
+  ClangTU = clang_parseTranslationUnit(Index, fileName.c_str(), Args, 1,
+   nullptr, 0, TUFlags);
+
+  Traverse([&](CXCursor cursor, CXCursor parent) -> CXChildVisitResult {
+if (cursor.kind != CXCursor_AnnotateAttr)
+  return CXChildVisit_Recurse;
+
+int childCount = 0;
+
+clang_visitChildren(
+cursor,
+[](CXCursor child, CXCursor, CXClientData data) -> CXChildVisitResult {
+  int *pcount = static_cast(data);
+
+  // we only expect one argument here, so bail
+  EXPECT_EQ(*pcount, 0);
+
+  auto *result = clang_Cursor_Evaluate(child);
+  EXPECT_NE(result, nullptr);
+  EXPECT_EQ(clang_EvalResult_getAsInt(result), 42);
+  ++*pcount;
+
+  return CXChildVisit_Recurse;
+},
+);
+
+return CXChildVisit_Continue;
+  });
+}
+
 class LibclangRewriteTest : public LibclangParseTest {
 public:
   CXRewriter Rew = nullptr;
Index: clang/tools/libclang/CursorVisitor.h
===
--- clang/tools/libclang/CursorVisitor.h
+++ clang/tools/libclang/CursorVisitor.h
@@ -276,7 +276,9 @@
   bool IsInRegionOfInterest(CXCursor C);
   bool RunVisitorWorkList(VisitorWorkList );
   void EnqueueWorkList(VisitorWorkList , const Stmt *S);
+  void EnqueueWorkList(VisitorWorkList , const Attr *A);
   LLVM_ATTRIBUTE_NOINLINE bool Visit(const Stmt *S);
+  LLVM_ATTRIBUTE_NOINLINE bool Visit(const Attr *A);
 
 private:
   std::optional handleDeclForVisitation(const Decl *D);
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -22,6 +22,7 @@
 #include "CursorVisitor.h"
 #include "clang-c/FatalErrorHandler.h"
 #include "clang/AST/Attr.h"
+#include "clang/AST/AttrVisitor.h"
 #include "clang/AST/DeclObjCCommon.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
@@ -526,6 +527,13 @@
 return false;
   }
 
+  if (clang_isAttribute(Cursor.kind)) {
+if (const Attr *A = getCursorAttr(Cursor))
+  return Visit(A);
+
+return false;
+  }
+
   if (clang_isTranslationUnit(Cursor.kind)) {
 CXTranslationUnit TU = getCursorTU(Cursor);
 ASTUnit *CXXUnit = cxtu::getASTUnit(TU);
@@ -2088,7 +2096,8 @@
 (SourceLocation::UIntTy)(uintptr_t)data[1]);
   }
 };
-class EnqueueVisitor : public ConstStmtVisitor {
+class EnqueueVisitor : public ConstStmtVisitor,
+   public ConstAttrVisitor {
   friend class OMPClauseEnqueue;
   VisitorWorkList 
   CXCursor Parent;
@@ -2230,6 +2239,10 @@
   void VisitOMPTargetTeamsDistributeSimdDirective(
   const OMPTargetTeamsDistributeSimdDirective *D);
 
+  // Attributes
+  void VisitAnnotateAttr(const AnnotateAttr *A);
+  void VisitAnnotateTypeAttr(const AnnotateTypeAttr *A);
+
 private:
   void AddDeclarationNameInfo(const Stmt *S);
   void AddNestedNameSpecifierLoc(NestedNameSpecifierLoc Qualifier);
@@ -2241,6 +2254,11 @@
   void AddTypeLoc(TypeSourceInfo *TI);
   void EnqueueChildren(const Stmt *S);
   void EnqueueChildren(const OMPClause *S);
+  template ::value ||
+ std::is_same::value,
+ bool> = true>
+  void EnqueueChildren(const AnnAttr *A);
 };
 } // namespace
 
@@ -2730,6 +2748,24 @@
   VisitorWorkList::iterator I = WL.begin() + size, E = WL.end();
   std::reverse(I, E);
 }
+
+template ::value ||
+   std::is_same::value,
+   bool>>
+void EnqueueVisitor::EnqueueChildren(const AnnAttr *A) {
+  unsigned size = WL.size();
+  for (const Expr *Arg : A->args()) {
+VisitStmt(Arg);
+  }
+  if (size == WL.size())
+return;
+  // Now reverse the entries we just added.  This will match the DFS
+  // ordering performed by the worklist.
+  VisitorWorkList::iterator I = WL.begin() + size, E = WL.end();
+  std::reverse(I, E);
+}
+

[PATCH] D151373: [libclang] Expose arguments of clang::annotate{_type}

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the additions! This should have test coverage in 
clang/unitttests/libclang/LibclangTest.cpp and a release note.




Comment at: clang/tools/libclang/CIndex.cpp:2745
 }
+// TODO these two methods are exactly the same. Can this be expressed better?
+void EnqueueVisitor::EnqueueChildren(const AnnotateAttr *A) {

We could use a template type and enable_if (for some type safety), then it 
shares the implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151373

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

[PATCH] D151373: [libclang] Expose arguments of clang::annotate{_type}

[Fridtjof Mund via Phabricator via cfe-commits]

fridtjof created this revision.
fridtjof added a reviewer: aaron.ballman.
Herald added a subscriber: arphaman.
Herald added a project: All.
fridtjof requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This enables easy consumption of arbitrary data added
to these annotations in addition to the annotation category,
which was already exposed.

I mostly just copied all the bits and pieces that seemed to do similar stuff, 
which seems to work.

I tested this locally with a very small C++ program to try and dump arguments 
for clang::annotate, which works.
Adding annotate_type to my test source code does not even expose the annotation 
itself however (I've attached it to parameter and variable types to no 
success), which seems like an unrelated bug (it should work for all Decls!).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151373

Files:
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CursorVisitor.h

Index: clang/tools/libclang/CursorVisitor.h
===
--- clang/tools/libclang/CursorVisitor.h
+++ clang/tools/libclang/CursorVisitor.h
@@ -276,7 +276,9 @@
   bool IsInRegionOfInterest(CXCursor C);
   bool RunVisitorWorkList(VisitorWorkList );
   void EnqueueWorkList(VisitorWorkList , const Stmt *S);
+  void EnqueueWorkList(VisitorWorkList , const Attr *A);
   LLVM_ATTRIBUTE_NOINLINE bool Visit(const Stmt *S);
+  LLVM_ATTRIBUTE_NOINLINE bool Visit(const Attr *A);
 
 private:
   std::optional handleDeclForVisitation(const Decl *D);
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -22,6 +22,7 @@
 #include "CursorVisitor.h"
 #include "clang-c/FatalErrorHandler.h"
 #include "clang/AST/Attr.h"
+#include "clang/AST/AttrVisitor.h"
 #include "clang/AST/DeclObjCCommon.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/OpenMPClause.h"
@@ -523,6 +524,13 @@
 return false;
   }
 
+  if (clang_isAttribute(Cursor.kind)) {
+if (const Attr *A = getCursorAttr(Cursor))
+  return Visit(A);
+
+return false;
+  }
+
   if (clang_isTranslationUnit(Cursor.kind)) {
 CXTranslationUnit TU = getCursorTU(Cursor);
 ASTUnit *CXXUnit = cxtu::getASTUnit(TU);
@@ -2085,7 +2093,8 @@
 (SourceLocation::UIntTy)(uintptr_t)data[1]);
   }
 };
-class EnqueueVisitor : public ConstStmtVisitor {
+class EnqueueVisitor : public ConstStmtVisitor,
+   public ConstAttrVisitor {
   friend class OMPClauseEnqueue;
   VisitorWorkList 
   CXCursor Parent;
@@ -2227,6 +2236,10 @@
   void VisitOMPTargetTeamsDistributeSimdDirective(
   const OMPTargetTeamsDistributeSimdDirective *D);
 
+  // Attributes
+  void VisitAnnotateAttr(const AnnotateAttr *A);
+  void VisitAnnotateTypeAttr(const AnnotateTypeAttr *A);
+
 private:
   void AddDeclarationNameInfo(const Stmt *S);
   void AddNestedNameSpecifierLoc(NestedNameSpecifierLoc Qualifier);
@@ -2238,6 +2251,8 @@
   void AddTypeLoc(TypeSourceInfo *TI);
   void EnqueueChildren(const Stmt *S);
   void EnqueueChildren(const OMPClause *S);
+  void EnqueueChildren(const AnnotateAttr *A);
+  void EnqueueChildren(const AnnotateTypeAttr *A);
 };
 } // namespace
 
@@ -2727,6 +2742,31 @@
   VisitorWorkList::iterator I = WL.begin() + size, E = WL.end();
   std::reverse(I, E);
 }
+// TODO these two methods are exactly the same. Can this be expressed better?
+void EnqueueVisitor::EnqueueChildren(const AnnotateAttr *A) {
+  unsigned size = WL.size();
+  for (const Expr *Arg : A->args()) {
+VisitStmt(Arg);
+  }
+  if (size == WL.size())
+return;
+  // Now reverse the entries we just added.  This will match the DFS
+  // ordering performed by the worklist.
+  VisitorWorkList::iterator I = WL.begin() + size, E = WL.end();
+  std::reverse(I, E);
+}
+void EnqueueVisitor::EnqueueChildren(const AnnotateTypeAttr *A) {
+  unsigned size = WL.size();
+  for (const Expr *Arg : A->args()) {
+AddStmt(Arg);
+  }
+  if (size == WL.size())
+return;
+  // Now reverse the entries we just added.  This will match the DFS
+  // ordering performed by the worklist.
+  VisitorWorkList::iterator I = WL.begin() + size, E = WL.end();
+  std::reverse(I, E);
+}
 void EnqueueVisitor::VisitAddrLabelExpr(const AddrLabelExpr *E) {
   WL.push_back(LabelRefVisit(E->getLabel(), E->getLabelLoc(), Parent));
 }
@@ -2999,7 +3039,7 @@
   // If the opaque value has a source expression, just transparently
   // visit that.  This is useful for (e.g.) pseudo-object expressions.
   if (Expr *SourceExpr = E->getSourceExpr())
-return Visit(SourceExpr);
+return ConstStmtVisitor::Visit(SourceExpr);
 }
 void EnqueueVisitor::VisitLambdaExpr(const LambdaExpr *E) {
   AddStmt(E->getBody());
@@ -3019,7 +3059,7 @@
 }
 void EnqueueVisitor::VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Treat the expression like its syntactic form.
-