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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a request for a comment to be added. Thank you!



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3706
+
+    if (!Result || !Notes.empty()) {
+      Diag(E->getBeginLoc(), diag::err_attribute_argument_n_type)
----------------
Tyker wrote:
> aaron.ballman wrote:
> > 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?
> AFAIK
> Result means the expression can be folded to a constant.
> Note.empty() means the expression is a valid constant expression in the current language mode.
> 
> the difference is observable in code like:
> ```
> constexpr int foldable_but_invalid() {
>   int *A = new int(0);
>   return *A;
> }
> 
> [[clang::annotate("", foldable_but_invalid())]] void f1() {}
> ```
> foldable_but_invalid retruns a constant but any constant evaluation of this function is invalid because it doesn't desallocate A.
> with !Notes.empty() this fails, without it no errors occurs.
> 
> i think it is desirable that attributes don't diverge from the language mode.
> I added a test for this.
Ah, thank you for explaining that! Can you add a comment to the code to make that clear for others who may run across it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645