mboehme marked 4 inline comments as done.
mboehme added a comment.
In D111548#3439039 <https://reviews.llvm.org/D111548#3439039>, @aaron.ballman
wrote:
> Also, you should add a release note about the new attribute
Done.
and check the Precommit CI pipeline failure out:
> Failed Tests (1):
>
> Clang :: CodeGenCXX/annotate-type.cpp
Fixed.
================
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.
----------------
aaron.ballman wrote:
> 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?
Good point. I've done what I think you're implying, namely that we should
remove the comments saying that the attribute won't have any any effect on the
LLVM IR?
================
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;
----------------
aaron.ballman wrote:
> 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.
So what's your position -- should I not print the attribute at all (which is
what I'm doing right now) or should I print it as something like
`[[clang::annotate_type(...)]]`?
================
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()),
----------------
aaron.ballman wrote:
> 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.
By "NFC change", you mean I could submit this separately as an NFC change?
Since this change is already part of this patch, I think I'd prefer to just
keep it in here, if that's OK?
================
Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
----------------
aaron.ballman wrote:
> 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.
I've added tests as you suggested, though I put most of them in
Sema/annotate-type.c, as they're not specific to C++.
Thanks for prompting me to do this -- the tests caused me to notice and fix a
number of bugs.
I haven't managed to diagnose the case when the attribute appears at the
beginning of the declaration. It seems to me that, at the point where I've
added the check, this case is indistinguishable from an attribute that appears
on the type. In both cases, the `TAL` is `TAL_DeclSpec`, and the attribute is
contained in `DeclSpec::getAttributes()`. This is because
`Parser::ParseSimpleDeclaration` forwards the declaration attributes to the
`DeclSpec`:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
I believe if I wanted to prohibit this case, I would need to add a check to
`Parser::ParseStatementOrDeclaration` and prohibit any occurrences of
`annotate_type` there. However, this seems the wrong place to do this because
it doesn't contain any specific processing for other attributes.
I've noticed that Clang also doesn't prohibit other type attributes (even ones
with C++ 11 syntax) from being applied to declarations. For example:
https://godbolt.org/z/Yj1zWY7nn
How do you think I should proceed here? I think the underlying issue is that
Clang doesn't always distinguish cleanly between declaration attributes and
type attributes, and fixing this might be nontrivial.
================
Comment at: clang/unittests/AST/AttrTest.cpp:39
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+ const std::string &Name) {
----------------
aaron.ballman wrote:
> 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
> ```
> Rather than doing the AST tests this way, you could add an -ast-dump to
> clang\test\AST and test the AST more directly.
I think a problem with this might be that `TypePrinter` can't print the
arguments of the attribute, and I want to test those?
> 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.
Done. I've also introduced a test helper `AssertAnnotatedAs` to keep the tests
as compact as I can.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111548/new/
https://reviews.llvm.org/D111548
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
