[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This is in good shape. Some relatively minor comments (plus there are some unrelated whitespace changes in a few files); the only nontrivial comment I have is that I think it'd be cleaner to cache `StringLiteral*`s rather than adding a new form of `LValueBase` for the case of a global constant string with no associataed `StringLiteral`.



================
Comment at: docs/LanguageExtensions.rst:2179
+Clang provides experimental builtins to support C++ standard library implementation
+of `std::experimental::source_location` as specified in  http://wg21.link/N4600.
+With the exception of `__builtin_COLUMN`, these builtins are also implemented by
----------------
rST uses double-backticks for code font. Does this URL get autolinked?


================
Comment at: include/clang/AST/APValue.h:66-73
+    LValueBase(const Expr *E, const char *StrVal,
+               const ConstantArrayType *ArrTy);
 
     template <class T>
-    LValueBase(T P, unsigned I = 0, unsigned V = 0)
-        : Ptr(P), CallIndex(I), Version(V) {}
+    LValueBase(T P, unsigned I = 0, unsigned V = 0) : Ptr(P), NormalData{I, V} {
+      assert(getBaseKind() == BK_Normal &&
+             "cannot create string base with this constructor");
----------------
This seems like a slightly dangerous overload set (eg, what does `LValueBase(P, 0, 0);` call when `P` is of type `Expr*`?)


================
Comment at: include/clang/AST/APValue.h:143-146
+    union {
+      NormalLValueData NormalData;
+      GlobalStringData StrData;
+    };
----------------
This makes `LValueBase` 8 bytes larger (on 64-bit targets) to support an uncommon case. Does `APValue` get larger too, or is its size dominated by something else? If needed, we could avoid the size increase by separately allocating the `GlobalStringData` and storing a pointer to it here -- and maybe something like a `GlobalStringData*` is what we should be caching on the `ASTContext` instead of a `const char*`?

However, as you mentioned in a prior comment:

> An alternative solution would be to create a `StringLiteral`, cache it in `ASTContext` for reuse, and return the new expression. Would this require serializing the `ASTContext` cache? Would this be a better solution?

Yes, I think that'd be a better and cleaner solution. We shouldn't need to serialize the `ASTContext` cache, because we never serialize `APValue`s. (And if we ever start doing so, serialization of lvalues denoting `Expr*`s becomes a hard problem in general, not only in this case.)

Naturally, you'll need to cache the strings used by `__builtin_FILE` in addition to the current cache for strings used by `__builtin_FUNCTION`, but on the plus side you should be able to reuse the existing code that generates `StringLiteral` objects for `PredefinedExpr`s (and maybe you could even share a cache between the new builtins and the `PredefinedExpr` handling?)


================
Comment at: lib/AST/APValue.cpp:46-49
+template <class T> static bool isEmptyOrTombstoneKey(T const &V) {
+  using DMI = llvm::DenseMapInfo<T>;
+  return V == DMI::getEmptyKey() || V == DMI::getTombstoneKey();
+}
----------------
This seems to be unused?


================
Comment at: lib/AST/ASTContext.cpp:9915
+    assert(D->getDeclName() && "expected declaration to have a name");
+    std::string S = D->getDeclName().getAsString();
+    Result = strcpy((char *)Allocate(S.size() + 1), S.c_str());
----------------
Please use `PredefinedExpr::ComputeName` here to reuse the same logic that we use for `__FUNCTION__`. We don't want `__FUNCTION__` and `__builtin_FUNCTION()` to return different strings for the same function.


================
Comment at: lib/AST/ExprConstant.cpp:72
+    if (B.isGlobalString())
+      return B.getGlobalStringType()->desugar();
+
----------------
Why do you need to desugar the type here? `getType` can generally return a sugared type.


================
Comment at: lib/AST/ExprConstant.cpp:3353
+      APValue Str(LVal.Base, CharUnits::Zero(), APValue::NoLValuePath(), 0);
+      CompleteObject StrObj(&Str, LVal.Base.getGlobalStringType()->desugar(),
+                            false);
----------------
Similarly, why do you need to desugar the type here?


================
Comment at: lib/CodeGen/CGExprConstant.cpp:867-869
-  llvm::Constant *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *DAE, QualType T) {
-    return Visit(DAE->getExpr(), T);
-  }
----------------
Why was this removed?


================
Comment at: lib/CodeGen/CGExprConstant.cpp:868-869
   llvm::Constant *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE, QualType T) {
+    // FIXME: Is it ever possible for a SourceLocExpr to be below this node?
+    // If so, we need to update the CurSourceLocExprScope in CodeGenFunction.
+
----------------
I think the salient point here is: this visitor will fail if it reaches a `SourceLocExpr`, so there's no need to track state that only it needs. (Same reason we didn't need a `CXXDefaultInitExprScope` for handling `CXXThisExpr`.)


================
Comment at: lib/CodeGen/CGExprConstant.cpp:1444-1445
   // are not allowed by tryEmitPrivateForMemory alone.
-  if (auto value = D.evaluateValue()) {
+  if (auto value = D.evaluateValue())
     return tryEmitPrivateForMemory(*value, destType);
 
----------------
Preferably commit this minor formatting cleanup separately.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:592
+
+    // else, we're building a string literal
+    const APValue::LValueBase &Base = Evaluated.getLValueBase();
----------------
Please comment in the form of a sentence :)


https://reviews.llvm.org/D37035