keryell added a comment.
Great feature for all the weird pieces of hardware all around the world! :-)
================
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),
----------------
Curious reindenting with mix-and-match of spaces & tabulations?
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2386-2390
+ llvm::ConstantExpr::getBitCast(ASZeroGV, Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(AnnoGV, Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(UnitGV, Int8PtrTy),
+ LineNoCst,
+ Args,
----------------
Same curious reindenting.
================
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:
> 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!
Perhaps a few comments on this clarification would be useful in the code for
the plain mortals...
================
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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88645/new/
https://reviews.llvm.org/D88645
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
