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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
