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

[Aaron Ballman via Phabricator via cfe-commits]

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