[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D129488#3762490 <https://reviews.llvm.org/D129488#3762490>, @jyknight wrote:

> The more I look at this, the more I feel like, if all the above behaviors are correct, source_location::current() simply shouldn't be marked consteval at all. If it was constexpr, instead, it'd work great _without_ needing any special-casing, because the "must evaluate immediately upon seeing function definition even in default arg position" wouldn't apply to it. And that's the behavior we //want// from it!

I think it's true that this would give the desired behavior without any hacks on the side of the compiler.
I am not sure why `consteval` was chosen in the first place, I suspect the goal was to reduce runtime costs.

I prototyped a change that forces `constexpr` on `current()` and it definitely works wrt to the scope rules.
If we choose this approach, Clang will not be standard-compliant with C++20. Raising this with WG21 is definitely something we need to do.
@aaron.ballman would you be able to discuss this within the WG21 or provide pointers on how these things are done? I am a total stranger to this process.

In the meantime, what options do we have for existing C++20 Standard and existing STL libraries?
The ones I see are:

1. Clang keeps the status quo and provides wrong results for `consteval source_location::current()`.
2. Clang internally switches "hacks up" `current()` to be `constexpr` even if it was marked as consteval, which means it can produce runtime calls and silently compile code where using `consteval` function `current()` would be an error.
3. Same as (2), but try to mitigate incompatibilities, e.g. lower calls to `current()` to constants, do not do the codegen the body of `current()`.
4. Keep `current()` consteval, but special-case calls to it.

(1) does not look like an option.
I am not sure entirely various problems that (2) and (3) would entail, but this only seems reasonable if WG21 decides `current()` must be changed to `constexpr` in the long run.
A compiler change for (2) is very simple, so I would vote for this if there are not problematic implications for users, e.g. what are the compatibility guarantees when linking together code compiled both with Clang and GCC? Would be break any promises Clang has around those?
If we are to choose between (3) and (4), I would definitely go with (4) as the compiler changes are roughly equivalent, but treating `current()` as `consteval` function produces exactly the diagnostics you expect from `consteval` (e.g. prevents taking a pointer to it).

I'd say we should choose between (2) and (4).
Prefer (2) if WG21 decides to switch to `constexpr` or we are certain there are no potential issues (see comment about GCC compatibility above).
Prefer (4) otherwise.

In D129488#3763252 <https://reviews.llvm.org/D129488#3763252>, @ChuanqiXu wrote:

> In D129488#3760982 <https://reviews.llvm.org/D129488#3760982>, @ilya-biryukov wrote:
>
>> @aaron.ballman, that's my reading of the standard as well. Do you think we should proceed with the current approach or is there another direction worth pursuing to make source_location work?
>>
>> In D129488#3760178 <https://reviews.llvm.org/D129488#3760178>, @ChuanqiXu wrote:
>>
>>> 
>
> It is just an example. I just wanted to say we could solve more problems if we chose that way. The issue of the module may not so painful and it shouldn't be hard to handle it specially. (Although specially handling looks not good, this is the reason why I didn't fix it). But after all, I am not objecting (nor approving) this for the modules example.

As mentioned earlier, I also think this might be the right call.
I just wanted to mention it's a shift against the current Clang design and exposes a few other issues that need to be worked out and also makes processing default arguments slower.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488