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

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3683
+  auto *Attr =
+      AnnotateAttr::Create(Context, Str, Args.empty() ? nullptr : Args.data(),
+                           Args.size(), CI.getRange(), CI.getSyntax());
----------------
aaron.ballman wrote:
> Just curious, but is the `?:` actually necessary? I would have assumed that an empty `ArrayRef` would return `nullptr` from `data()`.
maybe it was(in a previous version of the code) but not anymore since there is always at least one argument.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+    Expr *&E = Attr->args_begin()[Idx];
+    ConstantExpr *CE = dyn_cast<ConstantExpr>(E);
+    if (!E) {
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+      Result = E->EvaluateAsRValue(Eval, Context, true);
+    else
+      Result = E->EvaluateAsLValue(Eval, Context, true);
+
----------------
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.


================
Comment at: clang/test/SemaCXX/attr-annotate.cpp:1
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -verify %s
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645