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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
