[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight marked 8 inline comments as done.
jyknight added a comment.

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;
  ~~^



================
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).
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159