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

[Tyker via Phabricator via llvm-commits]

Tyker added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2229-2233
+  SmallVector<llvm::Value *, 5> Args = {
+      AnnotatedVal,
+      Builder.CreateBitCast(CGM.EmitAnnotationString(AnnotationStr), Int8PtrTy),
+      Builder.CreateBitCast(CGM.EmitAnnotationUnit(Location), Int8PtrTy),
+      CGM.EmitAnnotationLineNo(Location),
----------------
keryell wrote:
> Curious reindenting with mix-and-match of spaces & tabulations?
these are not tabs i believe this is just how phabricator shows indentation changes.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3693
+    if (E->isValueDependent() || E->isTypeDependent() ||
+        (CE && CE->hasAPValueResult()))
+      continue;
----------------
aaron.ballman wrote:
> What is the rationale for bailing out when the constant expression has an `APValue` result?
the intent was that during nested template instantiation arguement will be evaluated as soon as they are neither value nor type dependent and the result will be stored in the ConstantExpr. if an arguement has already been evaluated in a previous instantiation we don't want to evaluate it again.
but because of SubstExpr ConstantExpr are getting removed so it removed it.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+      Result = E->EvaluateAsRValue(Eval, Context, true);
+    else
+      Result = E->EvaluateAsLValue(Eval, Context, true);
+
----------------
keryell wrote:
> aaron.ballman wrote:
> > 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).
> This needs some motivating comments and explanations in the code.
> Also the variations on `g_i` annotation above are quite interesting and should appear in the unit tests. I am unsure they are now.
> Perhaps more interesting with other values than `0` in the example. :-)
> I guess we can still use `[[clang::annotate("test", &g_i)]] void t() {}` to have a pointer on `g_i` and `(int&)g_i` for reference? To add to the unit tests if not already checked.
> 
i changed to use EvaluateAsConstantExpr + LValueToRvalue cast.

> This needs some motivating comments and explanations in the code.
> Also the variations on `g_i` annotation above are quite interesting and should appear in the unit tests. I am unsure they are now.
they are tested not as expressively as above but they are tested.

> Perhaps more interesting with other values than `0` in the example. :-)
> I guess we can still use `[[clang::annotate("test", &g_i)]] void t() {}` to have a pointer on `g_i`
yes this is how it is working.
> `(int&)g_i` for reference? To add to the unit tests if not already checked.
(int&)g_i has the salve value category and same type as g_i it just has a noop cast around it.
pointer an reference are indistinguishable in an APValue the only difference is the c++ type of what they represent.
and they are of course lowered to the same thing. so we only need the pointer case.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645