aaron.ballman added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2338
+ Exprs = Exprs.drop_front();
+ if (Exprs.size() == 0)
+ return llvm::ConstantPointerNull::get(Int8PtrTy);
----------------
`Exprs.empty()` would be more concise.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2354-2356
+ auto *Constant = ConstEmiter.emitAbstract(
+ CE->getBeginLoc(), CE->getAPValueResult(), CE->getType());
+ return Constant;
----------------
You can return the expression directly, which removes a somewhat questionable
use of `auto`.
================
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());
----------------
Just curious, but is the `?:` actually necessary? I would have assumed that an
empty `ArrayRef` would return `nullptr` from `data()`.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+ Expr *&E = Attr->args_begin()[Idx];
+ ConstantExpr *CE = dyn_cast<ConstantExpr>(E);
+ if (!E) {
----------------
`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.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3691
+ Diag(CI.getLoc(), diag::err_attribute_argument_n_type)
+ << "'annotate'" << Idx << AANT_ArgumentConstantExpr;
+ return;
----------------
This is unfortunate -- you should be passing in the `ParsedAttr` object so that
the diagnostic engine can take care of properly formatting the diagnostic. For
instance, this will incorrectly name the attribute `'annotate'` instead of
`clang::annotate'` if that's how the user spelled the attribute. I don't recall
whether it will work on an `AttributeCommonInfo` though.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3694
+ }
+ if (E->getDependence() || (CE && CE->hasAPValueResult()))
+ continue;
----------------
`E->getDependence()` smells a bit fishy because this will early bail on any
kind of dependency, including an error dependency. Do you mean to care only
about type and/or value dependence here?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+ Result = E->EvaluateAsRValue(Eval, Context, true);
+ else
+ Result = E->EvaluateAsLValue(Eval, Context, true);
+
----------------
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.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3730
+ for (unsigned Idx = 0; Idx < AL.getNumArgs(); Idx++) {
+ if (AL.isArgIdent(Idx))
+ Args.push_back(nullptr);
----------------
Hmm, I think you should assert that you never get an identifier at this point
-- the parser should be treating all of these as expressions and not simple
identifiers.
================
Comment at: clang/test/SemaCXX/attr-annotate.cpp:1
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -verify %s
+
----------------
Do you mean to emit llvm here? I think that should probably be `-fsyntax-only`
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
