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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
