[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6409-6411
+- The attribute will not cause any additional data to be output to LLVM IR,
+  object files or debug information. It is only intended to be consumed using
+  Clang APIs by source-level static analysis tools such as Clang-Tidy.
----------------
It seems to me that because we found reasons for `annotate` to be lowered to LLVM IR, we'll find reasons to lower `annotate_type` eventually. I worry that if we document this, we'll never be able to modify the attribute to output information to LLVM IR because people will rely on it not doing so and be broken by a change. Are we sure we want to commit to this as part of the design?


================
Comment at: clang/lib/AST/TypePrinter.cpp:1686
+  // would require retrieving the attribute arguments, which we don't have here.
+  if (T->getAttrKind() == attr::AnnotateType)
+    return;
----------------
mboehme wrote:
> xazax.hun wrote:
> > Would it make sense to print something without the arguments? I wonder which behavior would be less confusing.
> TBH I'm not sure. Is TypePrinter used only to produce debug output or is it required that the output of TypePrinter will parse again correctly?
> 
> The reason I'm asking is because `annotate_type` has a mandatory first argument; if we need the output to be parseable, we would have to print some dummy string, e.g. `[[clang::annotate_type("arguments_omitted")]]`. This seems a bit strange, but maybe it's still worth doing. OTOH, if the output is used only for debugging, I guess we could print something like `[[clang::annotate_type(...)]]`, which doesn't parse but by that very nature makes it clear that we don't have all the information here.
> 
> I guess both of these options could have limited use -- they would at least make it clear that there is an annotation on the type, though without the arguments, we can't know what the actual annotation is.
> 
> Would appreciate some more input on the wider context here.
Yeah, the trouble is that you need a `TypeLoc` in order to get back to the actual attribute information that stores the arguments. But the type printer is printing types not specific instances of types at the location they're used.

The goal with the pretty printers is to print something back that the user actually wrote, but it's not always possible and so this is sort of a best-effort.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:396-408
+    if (E->getType()->isArrayType())
+      E = ImpCastExprToType(E, Context.getPointerType(E->getType()),
+                            clang::CK_ArrayToPointerDecay)
+              .get();
+    if (E->getType()->isFunctionType())
+      E = ImplicitCastExpr::Create(Context,
+                                   Context.getPointerType(E->getType()),
----------------
This seems an awful lot like doing `DefaultFunctionArrayLValueConversion()` here -- can you call that to do the heavy lifting?

Oh, I see we're just shuffling code around. Feel free to ignore or make as an NFC change if you'd prefer.


================
Comment at: clang/lib/Sema/SemaType.cpp:8102
+static void HandleAnnotateTypeAttr(TypeProcessingState &State,
+                                   QualType &CurType, const ParsedAttr &Attr) {
+  Sema &S = State.getSema();
----------------
Just to avoid re-using a type name as a declaration.


================
Comment at: clang/lib/Sema/SemaType.cpp:8117-8119
+  if (!S.ConstantFoldAttrArgs(Attr, Args)) {
+    return;
+  }
----------------



================
Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
----------------
Can you also add some test coverage for applying the attribute to a declaration instead of a type or not giving it any arguments? Also, should test arguments which are not a constant expression.


================
Comment at: clang/unittests/AST/AttrTest.cpp:39
 
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+                                    const std::string &Name) {
----------------
Rather than doing the AST tests this way, you could add an `-ast-dump` to `clang\test\AST` and test the AST more directly. Coverage is roughly the same, but the AST test may be easier to read.

Other AST things to test: more complicated types that have the attribute to ensure it shows up in the correct places and can be written on deduced types.
```
int [[attr]] * [[attr]] ident[10] [[attr]];
void (* [[attr]] fp)(void);
__auto_type [[attr]] oooh = 12; // in C mode

struct S { int mem; };
int [[attr]] S::* [[attr]] oye = &S::mem; // in C++ mode
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111548