jyknight added a comment. 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. ================ 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 ---------------- 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. ================ Comment at: clang/include/clang/AST/DeclCXX.h:4192 +/// An artificial decl, representing am global anonymous constant value which is +/// uniquified by value within a compilation. +/// ---------------- aaron.ballman wrote: > I presume this is what was meant, right? Yep. ================ Comment at: clang/include/clang/AST/DeclCXX.h:4221 + /// Print this in a human-readable format. + void printName(llvm::raw_ostream &OS) const override; + ---------------- 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. ================ Comment at: clang/include/clang/AST/Expr.h:4709 - bool isStringType() const { + bool isIntType() const { switch (getIdentKind()) { ---------------- 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. ================ Comment at: clang/include/clang/Serialization/ASTBitCodes.h:44 /// AST files at this time. const unsigned VERSION_MAJOR = 15; ---------------- aaron.ballman wrote: > Do we need to bump this value now that we have a new kind of AST node? Probably so. It looks like we don't generally bump this number, but that's probably accidental. ================ Comment at: clang/lib/AST/Expr.cpp:2270 + // __builtin_FUNCTION() above returns! + const Decl *CurDecl = dyn_cast_or_null<Decl>(Context); + Value.getStructField(F->getFieldIndex()) = MakeStringLiteral( ---------------- aaron.ballman wrote: > Under what conditions can `Context` be null? (should this be a `dyn_cast` > instead?) That came from the similar code above for __builtin_FUNCTION. I don't think it can actually be null (in either place). Changed in both. ================ 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: > 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...) ================ Comment at: clang/lib/Sema/SemaExpr.cpp:3405 + case Decl::UnnamedGlobalConstant: + valueKind = VK_LValue; + break; ---------------- 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. ================ Comment at: clang/test/SemaCXX/source_location.cpp:383-385 + // static_assert(is_equal(Default.info.function(), "TestCtor<>::TestCtor()")); + static_assert(is_equal(Default.ctor_info.function(), "")); + // static_assert(is_equal(Template.info.function(), "TestCtor<>::TestCtor(int, U) [U = std::source_location]")); ---------------- aaron.ballman wrote: > Why are these commented out? Oops -- intended to update those to the proper strings. Fixed. 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
