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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> I was also under impression that this should fall out of the C++ semantics, but after playing around with it I am not so sure anymore.

I'm also starting to question that. :-)

>> Isn't it wrong to eagerly evaluate _other_ calls in default args, as well?
>> ISTM that source_location is simply _exposing_ the bug in where we evaluate these expressions, but that it's actually a more general problem?
>
> Yes, it will evaluate calls to other `consteval` functions and and I don't think there is a way to notice this if there are no errors in the `consteval` functions. `constexpr` and `consteval` functions should produce the same value when called with the same argument IIUC.
> `source_location::current()` is special in that regard and it breaks that assumption. E.g. the fact that `CXXDefaultArgExpr` references the same expression from `ParmVarDecl`.
> We could delay evaluation of other `consteval` functions until the default argument is actually used relatively cheaply for other functions, but `source_location::current()` is the only one that requires a complete revamp of how `CXXDefaultArgExpr` works (see D132941 <https://reviews.llvm.org/D132941> for an attempt, it's not pretty, unless there is a simple way I missed).
>
> I am also torn on how to interpret the standard. It might be that evaluating immediate function in default arguments in actually required.
> `[dcl.fct.default]p5` mentions we need to do full semantic checking of the initializer for default arguments:
>
>   The default argument has the same semantic constraints as the initializer in a declaration of a variable of the
>   parameter type, using the copy-initialization semantics (9.4). The names in the default argument are bound,
>   and the semantic constraints are checked, at the point where the default argument appears.
>
> Additionally, the standard requires all immediate invocations to be evaluated (unless they are in immediate function context, which is not the case if default arguments are not for consteval function).
> Hence, we should also evaluate the immediate invocations as part of the semantic checking we do for initializer of the default argument.
>
> All major compilers certainly evaluate the `consteval` functions in the default arguments even without calls to the function that has those parameters:
> https://gcc.godbolt.org/z/qTzrf6Msx
>
> It might be an implementation quirk and a clarification would be nice. I am leaning towards thinking this behavior is actually standard-compliant.
> This does not contradict the fact that default arguments must be evaluated in the context of the caller.
> One can't see the difference for `consteval` functions as they must be replaced by constants.
>
> Both GCC and MSVC seem to special-case `std::source_location::current()` from what I can tell.

https://eel.is/c++draft/expr.call#7.sentence-3 is what causes the argument to be evaluated at the call expression rather than at the declaration while https://eel.is/c++draft/dcl.fct.default#5 makes it clear that the semantic constraints are checked at the declaration location. I couldn't find explicit wording which says this, but I believe that semantic checking in this case also involves determining whether the immediate function is a valid or not (https://eel.is/c++draft/expr.const#14), which should explain the behavior of your example in https://gcc.godbolt.org/z/qTzrf6Msx.

However, https://eel.is/c++draft/dcl.constexpr#5 says that the behavior has to be the same whether the constexpr function is runtime evaluated or constant evaluated, and a function declared `consteval` is a constexpr function (https://eel.is/c++draft/dcl.constexpr#2.sentence-1). That sure does make std::source_location::current() a rather special function -- if the call were to be evaluated at runtime (which it can't because it's an immediate function), it would give different results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488