[PATCH] D136554: Implement CWG2631

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:507-508
   ``-std=gnu++14`` to their build settings to restore the previous behaviour.
+- Implemented DR2631. Invalid ``consteval`` calls in default arguments and default
+  member initializers are diagnosed when and if the default is used.
 
----------------
cor3ntin wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Should this be listed as a potentially breaking change as this now 1) potentially changes the value people were getting for `source_location::current()` as a default argument, and 2) potentially causes instantiation differences due to the change in ODR use?
> > No because the current value of `source_location::current()` is invalid per library wording ( i should reference the issue though) and 2/ i don't think there are any difference in odr use except the point of diagnostic. Or at least i don't think you could observe it?
> The wording for source location
> http://eel.is/c++draft/support.srcloc#cons-2 
> 
> Without that issue resolution this calls for magic.
That's correct from a standards perspective, but I'm talking about the reality which is that this is a behavioral change that our users could notice and could impact code outside of library calls too.

FWIW, even with this issue resolution, library is calling for magic (magical thinking) IMO -- it's a constant evaluated call that doesn't produce constant results and thus isn't suitable to be called immediately... even in an immediate context... despite being called an immediate function.

But thinking about it more... I think we don't need to call it out as a potentially breaking change. Real consteval calls give the same answer every time you call them with the same input, so this really should only impact people using `source_location::current()` because that doesn't match the consteval model.


================
Comment at: clang/test/CXX/class/class.local/p1-0x.cpp:14
       int& x2 = x; // expected-error{{reference to local variable 'x' declared in enclosing lambda expression}}
-    };
+    }c; // expected-note {{required here}}
   };
----------------
cor3ntin wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Double-checking: you did intend to name that local variable, right?
> > Yes, that's actually the change I'm talking about,
> > That specific warning only triggers when the initializer is ODR used, which now only happens when a constructor is defined, which, in the case of aggregate, only happens on use of said aggregate.
> Note that i considered delaying that warning only in the presence of immediate invocations but I think this would be inconsistent and surprising - and also probably wrong to mark things odr used if they are not actually ever used.
> And alternative would be to not tie that check with the odr-used marking, but that does seem like a pretty consequent change.
Ah okay, that makes sense, thank you for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554