aaron.ballman added a comment. In D120159#3348927 <https://reviews.llvm.org/D120159#3348927>, @jyknight wrote:
> In D120159#3341224 <https://reviews.llvm.org/D120159#3341224>, @aaron.ballman > wrote: > >> Btw, I think there may be some functionality missing for AST dumping, so I'd >> like to see some additional tests for that. > > I'm not sure what test would actually show a problem (or lack thereof) in the > ast dumping -- AFAICT, UnnamedGlobalConstantDecl can't actually show up in an > AST dump at the moment. The AST generated for a `__builtin_source_location` > call doesn't contain one, since it's only when evaluating the value that you > receive an LValue pointing to an UnnamedGlobalConstantDecl. But > https://github.com/llvm/llvm-project/blob/fdfe26ddbeb1a5eb6106a6a27d6f8240deca543f/clang/lib/AST/TextNodeDumper.cpp#L535 > doesn't print those usefully, anyhow. 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? ================ Comment at: clang/docs/LanguageExtensions.rst:3343-3344 +defined, and must contain exactly four fields: ``const char *_M_file_name``, +``const char *_M_function_name``, ``<any-integral-type> _M_line``, and +``<any-integral-type> _M_column``. The fields will be populated in the same +manner as the above four builtins, except that ``_M_function_name`` is populated ---------------- jyknight wrote: > aaron.ballman wrote: > > Doesn't the type for `<any-integral-type>` matter though, due to layout > > differences based on the size of the integer type picked? > > > > Also, a downside of requiring this type to be defined before the builtin > > can be used is that this builtin would otherwise be suitable in C, but > > there's no way to name that particular type. Is there anything we can do to > > keep this builtin generic enough to be used in C as well? > The type of the integer (as well as ordering of the fields) doesn't matter, > because the code populates fields in the actual struct layout as defined, it > doesn't assume a particular layout. > > As for making it usable from C code, possibly we could have it attempt to > look-up a `__source_location_impl` type in C mode (or maybe always, as a > fallback if std::source_location::__impl doesn't exist). > > But, that's something we could add later if needed -- and before going that > route, I think we should discuss with GCC/libstdc++ to try to remain > compatible. Okay, I'm fine adding that support later; I definitely agree about the compatibility concerns. ================ Comment at: clang/include/clang/AST/DeclCXX.h:4221 + /// Print this in a human-readable format. + void printName(llvm::raw_ostream &OS) const override; + ---------------- jyknight wrote: > aaron.ballman wrote: > > `printName()` in a class named `UnnamedGlobalConstantDecl` (which isn't a > > `NamedDecl`) seems a bit confusing as to what this actually prints. > > > > Also, what is this overriding? `NamedDecl` has a virtual `printName()` > > function, but `ValueDecl` does not. I have the sneaking suspicion that this > > can be removed. > `class ValueDecl : public NamedDecl` -- so, UnnamedGlobalConstantDecl _is_ a > "NamedDecl". I suspect it's not strictly needed, but it does affect what's > printed in debug output for this type. How the heck did I miss that? Sorry for the noise. ================ Comment at: clang/include/clang/AST/Expr.h:4709 - bool isStringType() const { + bool isIntType() const { switch (getIdentKind()) { ---------------- jyknight wrote: > aaron.ballman wrote: > > Should this also be marked `LLVM_READONLY` as in the original interface? > I don't think it'd actually make a difference, and we don't typically go > around annotating everything with that attribute. It was weird that > isStringType wasn't annotated but isIntType was, in the first place. SGTM ================ 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: > > 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. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:3405 + case Decl::UnnamedGlobalConstant: + valueKind = VK_LValue; + break; ---------------- jyknight wrote: > aaron.ballman wrote: > > Perhaps stupid question, but do we want this to be an lvalue as opposed to > > a prvalue? > I think so? I was thinking of it as acting effectively like a string literal. I was thinking the same thing, but I had thought string literals were a prvalue the same as integer literals, oops. :-) 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
