[PATCH] D136554: Implement CWG2631

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 20 inline comments as done.
cor3ntin 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.
 
----------------
aaron.ballman wrote:
> 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.
I think there are several things to consider.

* This is a bug fix https://github.com/llvm/llvm-project/issues/56379
* libc++ does not ship `<source_location>` (specifically because of this bug)
* `<experimental/source_location>` is not affected (because the current call is not `consteval`)

I think we should call out the issue either way though.



================
Comment at: clang/test/SemaCXX/cxx2a-consteval-default-params.cpp:29-30
+        return defaulted;
+    }()  // expected-note {{in the default initalizer of 'defaulted'}}
+) {
+    return 0;
----------------
aaron.ballman wrote:
> I think it'd be useful to show that use in an unevaluated context still works.
agreed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554