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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3696-3697
+
+    if (E->isValueDependent() || E->isTypeDependent())
+      continue;
+
----------------
Should this move up above the point where we're checking whether the expression has an array type?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3706
+
+    if (!Result || !Notes.empty()) {
+      Diag(E->getBeginLoc(), diag::err_attribute_argument_n_type)
----------------
I'm surprised that the presence of notes alone would mean the attribute argument has an error and should be skipped. Are there circumstances under which `Result` is true but `Notes` is non-empty?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3693
+    if (E->isValueDependent() || E->isTypeDependent() ||
+        (CE && CE->hasAPValueResult()))
+      continue;
----------------
Tyker wrote:
> 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.
Ah, thank you for the explanation and change.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:195
+      return;
+    else
+      Args.push_back(Result.get());
----------------
No `else` after a `return`.


================
Comment at: clang/test/SemaCXX/attr-annotate.cpp:96
+}
+
+void test() {}
----------------
Some more tests for various constant expression situations that may be weird:
```
int n = 10;
int vla[n];

[[clang::annotate("vlas are awful", sizeof(vla[++n]))] int i = 0; // reject, the sizeof is not unevaluated

[[clang::annotate("_Generic selection expression should be fine", _Generic(n, int : 0, default : 1))]] int j = 0; // second arg should resolve to 0 fine

void designator();
[[clang::annotate("function designators?", designator)]] int k = 0; // Should work?
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645