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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 10 inline comments as done.
EricWF added inline comments.


================
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");
----------------
rsmith wrote:
> This seems like a slightly dangerous overload set (eg, what does `LValueBase(P, 0, 0);` call when `P` is of type `Expr*`?)
Removed in place of the string literal caching implementation.


================
Comment at: include/clang/AST/APValue.h:143-146
+    union {
+      NormalLValueData NormalData;
+      GlobalStringData StrData;
+    };
----------------
rsmith wrote:
> 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?)
> Yes, I think that'd be a better and cleaner solution. We shouldn't need to serialize the ASTContext cache, because we never serialize APValues. (And if we ever start doing so, serialization of lvalues denoting Expr*s becomes a hard problem in general, not only in this case.)

SGTM. I've implemented this instead.

> on the plus side you should be able to reuse the existing code that generates StringLiteral objects for PredefinedExprs (and maybe you could even share a cache between the new builtins and the PredefinedExpr handling?)

The only issue with caching, and in particular adding it to `PredefinedExpr`, is that the `StringLiteral` no longer has meaningful location information. Is this OK? 


https://reviews.llvm.org/D37035