[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88645#2321980 <https://reviews.llvm.org/D88645#2321980>, @jdoerfert wrote:

> (Drive By: This is cool)

I didn't say this before, but yeah, I agree -- this is really quite neat, thank you for working on it!



================
Comment at: clang/include/clang/Sema/ParsedAttr.h:1077
+/// AttributeCommonInfo has a non-explicit constructor which takes an
+/// SourceRange as only argument, this constructor has many uses so making it
+/// explicit is hard. This constructor causes ambiguity with
----------------
as only argument -> as its only argument


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:1081
+/// So we use SFINAE to disable any conversion and remove any
+/// ambiguity.
+template <typename ACI,
----------------
The comment can probably be re-flowed so "ambiguity" is on the preceding line?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3693
+    if (E->isValueDependent() || E->isTypeDependent() ||
+        (CE && CE->hasAPValueResult()))
+      continue;
----------------
What is the rationale for bailing out when the constant expression has an `APValue` result?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+    Expr *&E = Attr->args_begin()[Idx];
+    ConstantExpr *CE = dyn_cast<ConstantExpr>(E);
+    if (!E) {
----------------
Tyker wrote:
> aaron.ballman wrote:
> > `auto *` since the type is spelled out in the initialization.
> > 
> > Also, I think this is unsafe -- it looks like during template instantiation, any arguments that have a substitution failure will come through as `nullptr`, and this will crash.
> > 
> > What should happen on template instantiation failure for these arguments? I think the whole attribute should probably be dropped with appropriate diagnostics rather than emitting a partial attribute, but perhaps you have different ideas.
> When template instantation fails nullptr is put in the Expr arguments and  AddAnnotationAttr will emit a diagnostic and not associate the attribut to the declaration.
Great, thank you for the fix!


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+      Result = E->EvaluateAsRValue(Eval, Context, true);
+    else
+      Result = E->EvaluateAsLValue(Eval, Context, true);
+
----------------
Tyker wrote:
> aaron.ballman wrote:
> > Under what circumstances would we want the constant expressions to be lvalues? I would have guessed you would want to call `Expr::EvaluateAsConstantExpr()` instead of either of these.
> Expr::EvaluateAsConstantExpr will evaluate expression in there value category.
> this can be quite surprising in some situations like:
> ```
> const int g_i = 0;
> [[clang::annotate("test", g_i)]] void t() {} // annotation carries a pointer/reference on g_i
> [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries the value 0
> [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries the value 0
> 
> ```
> with EvaluateAsRValue in all of the cases above the annotation will carry the value 0.
> 
> optionally we could wrap expression with an LValue to RValue cast and call EvaluateAsConstantExpr this should also work.
Thank you for the explanation. I think wrapping with an lvalue to rvalue conversion may make more sense -- `EvaluateAsRValue()` tries really hard to get an rvalue out of something even if the standard says that's not okay. It'll automatically apply the lvalue to rvalue conversion if appropriate, but it'll also do more than that (such as evaluating side effects).


================
Comment at: clang/test/SemaCXX/attr-annotate.cpp:1
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -verify %s
+
----------------
Tyker wrote:
> aaron.ballman wrote:
> > Do you mean to emit llvm here? I think that should probably be `-fsyntax-only`
> The reason i put an -emit-llvm is to also test the asserts in IRgen on more complex code.
Tests in sema shouldn't typically run codegen, so I'd rather this was `-fsyntax-only` and test the assertions more explicitly in a codegen-specific test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645