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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D129488#3761478 <https://reviews.llvm.org/D129488#3761478>, @ilya-biryukov wrote:

> In D129488#3761398 <https://reviews.llvm.org/D129488#3761398>, @jyknight wrote:
>
>> I believe this should print `fn=14 fn2=14`. I don't see how we can get that by special-casing calls to `std::source_location::current`, and indeed, with this version of the patch, the program instead prints `fn=14 fn2=9`.
>
> There is another issue in Clang that prevents it from producing correct results with this patch. We should not evaluate immediate calls inside default arguments of immediate function parameters as they are deemed to be inside the immediate function context <https://eel.is/c++draft/expr.const#14.1>. Clang currently evaluates these calls as all parameter scopes are `PotentiallyEvaluatedIfUsed` and `ImmediateFunctionContext` is a different value inside the same enumeration :)

OK...So...

If the correct rule is that consteval functions must be immediately evaluated even when used in a default arg of a non-consteval function (rather than deferring evaluation to the callsite, as would otherwise be the rule for default args!), then GCC's behavior for "fn3" in your example https://gcc.godbolt.org/z/P1x8PGsh6 makes sense. And, yea, also Clang's _current_ behavior for `current()` makes sense! Because `current` is not actually generating the source location itself -- the //actual// source location is coming from a builtin call in a default arg of `current()`...it's only a wrapper, just like `fn()` is a wrapper around `current()`.

But, "makes sense" (from implementation point of view) doesn't mean "useful"...which is why you're adding a special case for it. And why GCC also did.

This is really quite awful.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488