[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

In D120159#3349271 <https://reviews.llvm.org/D120159#3349271>, @jyknight wrote:

> In D120159#3349069 <https://reviews.llvm.org/D120159#3349069>, @aaron.ballman wrote:
>
>> Ah, okay, that's a good point. I was thinking this would show up in the AST in places like:
>>
>>   template <typename Ty>
>>   auto func() -> Ty;
>>   
>>   int main() {
>>     func<decltype(__builtin_source_location())>();
>>   }
>>
>> when we go to print the `Ty` type from the template. Does that not happen?
>
>
>
>   |-FunctionTemplateDecl 0x10ef3288 <line:11:1, line:12:16> col:6 func
>   | |-TemplateTypeParmDecl 0x10ef3068 <line:11:11, col:20> col:20 referenced typename depth 0 index 0 Ty
>   | |-FunctionDecl 0x10ef31e8 <line:12:1, col:16> col:6 func 'auto () -> Ty'
>   | `-FunctionDecl 0x10ef36d8 <col:1, col:16> col:6 used func 'auto () -> const std::source_location::__impl *'
>   |   `-TemplateArgument type 'const std::source_location::__impl *'
>   |     `-PointerType 0x10ef34c0 'const std::source_location::__impl *'
>   |       `-QualType 0x10ef2d81 'const std::source_location::__impl' const
>   |         `-RecordType 0x10ef2d80 'std::source_location::__impl'
>   |           `-CXXRecord 0x10ef2ce8 '__impl'
>   `-FunctionDecl 0x10ef33f0 <line:14:1, line:16:1> line:14:5 main 'int ()'
>     `-CompoundStmt 0x10ef38c0 <col:12, line:16:1>
>       `-CallExpr 0x10ef38a0 <line:15:3, col:47> 'const std::source_location::__impl *':'const std::source_location::__impl *'
>         `-ImplicitCastExpr 0x10ef3888 <col:3, col:45> 'auto (*)() -> const std::source_location::__impl *' <FunctionToPointerDecay>
>           `-DeclRefExpr 0x10ef37d0 <col:3, col:45> 'auto () -> const std::source_location::__impl *' lvalue Function 0x10ef36d8 'func' 'auto () -> const std::source_location::__impl *' (FunctionTemplate 0x10ef3288 'func')
>
> The type is simply `const std::source_location::__impl*`. Only the evaluated value refers to an UnnamedGlobalConstantDecl.
>
> If it was possible to use in a non-type template argument, I think you could get it to show up in the AST for the instantiation, but that's not supported. E.g.
>
>   template <auto T = __builtin_source_location()>
>       class X {};
>   
>   X<> x;
>
> Results in:
>
>   test.cc:12:20: error: non-type template argument does not refer to any declaration
>   template <auto T = __builtin_source_location()>
>                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   test.cc:15:3: note: while checking a default template argument used here
>   X<> x;
>   ~~^

Excellent, thank you for double-checking! This does raise an interesting question about the AST import functionality -- if `UnnamedGlobalConstantDecl` can't show up in the AST, do we need to handle AST reading and writing for it, or can we assert that it should never show up through PCH/Modules?

In general, this is looking pretty good to me. Adding Erich in case he spots something I missed.



================
Comment at: clang/lib/AST/ExprConstant.cpp:8830-8832
+      // (GCC apparently doesn't diagnose casts from void* to T* in constant
+      // evaluation, regardless of the types of the object or pointer; a
+      // subsequent access through the wrong type is diagnosed, however).
----------------
jyknight wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > aaron.ballman wrote:
> > > > Do we want to be even more restrictive here and tie it to libstdc++ (e.g., like we did in https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L12352)
> > > I don't see anything in the code you linked that makes it libstdc++-tied? But, in any case, I think restricting to std::source_location::current is sufficiently narrow that it doesn't much matter.
> > > 
> > > We also have the option of not doing this hack, since it's going to be fixed in libstdc++. However, given that a similar hack is already present for std::allocator, extending it to source_location doesn't seem that terrible. 
> > > 
> > > (And IMO, it would make sense for the C++ standard to actually permit this in constant evaluation, anyways...)
> > I'm fine adding the hack, but the restrictions I was thinking of was forcing it to only work in a system header.
> Oh, I see. That doesn't seem important here, since non-system code should not be defining a std::source_location::current() function anyhow.
> 
> (But if we keep needing to add exceptions here, maybe eventually we just decide to just turn this error into a default-on warningerror, and not diagnose it in system headers.)
okay, that's fair.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159