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