[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for this! I've got some initial comments on it.

Btw, I think there may be some functionality missing for AST dumping, so I'd like to see some additional tests for that.



================
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
----------------
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?


================
Comment at: clang/include/clang/AST/DeclCXX.h:4191
 
+/// An artificial decl, representing am global anonymous constant value which is
+/// uniquified by value within a compilation.
----------------



================
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.
+///
----------------
I presume this is what was meant, right?


================
Comment at: clang/include/clang/AST/DeclCXX.h:4221
+  /// Print this in a human-readable format.
+  void printName(llvm::raw_ostream &OS) const override;
+
----------------
`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.


================
Comment at: clang/include/clang/AST/Expr.h:4709
 
-  bool isStringType() const {
+  bool isIntType() const {
     switch (getIdentKind()) {
----------------
Should this also be marked `LLVM_READONLY` as in the original interface?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11506-11509
+def err_std_source_location_impl_not_found : Error<
+  "std::source_location::__impl was not found; it must be defined before __builtin_source_location is called.">;
+def err_std_source_location_impl_malformed : Error<
+  "std::source_location::__impl must be standard-layout and have only two 'const char *' fields _M_file_name and _M_function_name, and two integral fields _M_line and _M_column.">;
----------------



================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:44
 /// AST files at this time.
 const unsigned VERSION_MAJOR = 15;
 
----------------
Do we need to bump this value now that we have a new kind of AST node?


================
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(
----------------
Under what conditions can `Context` be null? (should this be a `dyn_cast` instead?)


================
Comment at: clang/lib/AST/ExprClassification.cpp:468-471
     islvalue = isa<VarDecl>(D) || isa<FieldDecl>(D) ||
-               isa<IndirectFieldDecl>(D) ||
-               isa<BindingDecl>(D) ||
-               isa<MSGuidDecl>(D) ||
+               isa<IndirectFieldDecl>(D) || isa<BindingDecl>(D) ||
+               isa<MSGuidDecl>(D) || isa<UnnamedGlobalConstantDecl>(D) ||
                isa<TemplateParamObjectDecl>(D) ||
----------------
Plus fix the formatting that I'm sure I broke.


================
Comment at: clang/lib/AST/ExprClassification.cpp:473-474
                (Ctx.getLangOpts().CPlusPlus &&
                 (isa<FunctionDecl>(D) || isa<MSPropertyDecl>(D) ||
                  isa<FunctionTemplateDecl>(D)));
 
----------------



================
Comment at: clang/lib/AST/ExprConstant.cpp:1982-1983
+    // ... the address of an unnamed global constant
+    return isa<FunctionDecl>(D) || isa<MSGuidDecl>(D) ||
+           isa<UnnamedGlobalConstantDecl>(D);
   }
----------------



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


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3257
 
-  // Enum constants are always r-values and never references.
+    // Enum constants are always r-values and never references.
   // Unresolved using declarations are dependent.
----------------
Spurious indentation change?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3405
+  case Decl::UnnamedGlobalConstant:
+    valueKind = VK_LValue;
+    break;
----------------
Perhaps stupid question, but do we want this to be an lvalue as opposed to a prvalue?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13831-13833
     } else if (!isa<FunctionDecl>(dcl) && !isa<NonTypeTemplateParmDecl>(dcl) &&
-               !isa<BindingDecl>(dcl) && !isa<MSGuidDecl>(dcl))
+               !isa<BindingDecl>(dcl) && !isa<MSGuidDecl>(dcl) &&
+               !isa<UnnamedGlobalConstantDecl>(dcl))
----------------
Might as well combine these as well.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16025
+    if (S.LookupQualifiedName(ResultSL, Std)) {
+      if (RecordDecl *SLDecl = ResultSL.getAsSingle<RecordDecl>()) {
+        LookupResult ResultImpl(S, &S.PP.getIdentifierTable().get("__impl"),
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:16072
+    } else {
+      Count = 0;
+      break;
----------------
I would set the count to 5 instead of 0 because otherwise someone could do something stupid like:
```
namespace std {
struct source_location {
  struct __impl {
    int Muahahahahhahaha;
    // ... The other four fields.
  };
};
}
```


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:6998-6999
       auto *VD = const_cast<ValueDecl *>(Base.dyn_cast<const ValueDecl *>());
-      if (Base && (!VD || isa<LifetimeExtendedTemporaryDecl>(VD))) {
+      if (Base && (!VD || isa<LifetimeExtendedTemporaryDecl>(VD) ||
+                   isa<UnnamedGlobalConstantDecl>(VD))) {
         Diag(Arg->getBeginLoc(), diag::err_template_arg_not_decl_ref)
----------------



================
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]"));
----------------
Why are these commented out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159