[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

Thu Feb 23 06:22:52 PST 2023

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM

================
Comment at: clang/test/SemaCXX/lambda-expressions.cpp:675-676
+StringLiteral(const char (&array)[N])
+    __attribute__((enable_if(__builtin_strlen(array) == 2,
+                              "invalid string literal")));
+};
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > 1) This code already compiles today without issue (https://godbolt.org/z/q6hPxoWq9), so are you sure this is testing what needs to be tested?
> > > > 
> > > > 2) What about for non-GNU-style attributes ([[]] attributes that appertain to the function type)? e.g.,
> > > > ```
> > > > template <int N>
> > > > void foo(const char (&array)[N]) [[clang::annotate_type("test", array)]];
> > > > ```
> > > Yes, this is something that was broken by this patch.
> > > enable_if did create odr uses (incorrectly). I can add a test for annotate_type but I don't think it would run into the same issue at all. 
> > So I'm not certain what's being tested then -- how are you validating that `array` is no longer ODR used?
> > 
> > In terms of `annotate_type`, that attribute accepts a string literal followed by a variable number of expression arguments, so I think it should have the same issues as a GNU-style attribute accepting an expression argument. I was mostly trying to wrap my head around whether `[[]]` and `__attribute__` are expected to have different behaviors when written in that position.
> Context https://reviews.llvm.org/D124351#4101611
I talked to @cor3ntin off-list to get a better understanding of the test situation, and I realized the issue was that this code used to give a diagnostic that it shouldn't have, so this is demonstrating we accept the code rather than reject it. That makes a lot more sense now!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124351