EricWF marked 18 inline comments as done.
EricWF added a comment.
Mark more review comments as done.
================
Comment at: lib/AST/Expr.cpp:2026-2027
+ // human readable name.
+ if (IdentifierInfo *II = FD->getDeclName().getAsIdentifierInfo())
+ return MkStr(II->getNameStart());
+ return MkStr(PredefinedExpr::ComputeName(PredefinedExpr::Function, FD));
----------------
rsmith wrote:
> Is the benefit of avoiding a `std::string` allocation here really worth the
> cost of duplicating this portion of the `__FUNCTION__` logic?
>
> If we care about the extra string allocation, it'd seem more worthwhile to
> extend `ComputeName` so that a stack-allocated `SmallString<N>` buffer can be
> passed in (eg, `StringRef ComputeName(..., SmallVectorImpl<char>
> &NameStorage)`, where `NameStorage` may, but need not, be used to store the
> resulting string).
Probably. not. I think this optimization attempt is a remnant from an older
caching approach.
================
Comment at: lib/AST/ExprConstant.cpp:2653-2655
+ APValue::LValueBase const &Base,
uint64_t Index) {
+ const Expr *Lit = Base.get<const Expr *>();
----------------
rsmith wrote:
> Hm, why the move of the `get` from caller to callee? This widens the set of
> possible arguments to `extractStringLiteralCharacter`, but all the new
> possibilities result in crashes -- shouldn't the caller, rather than the
> callee, still be the place where we house the assertion that the base is
> indeed an expression?
Remant of an older version of the patch. I'll revert.
Sorry, I'll try being better about reviewing patches for unnecessary changes.
================
Comment at: lib/AST/ExprConstant.cpp:3370-3371
+
+ assert((!Base || !isa<SourceLocExpr>(Base)) &&
+ "Base should have already been transformed into a StringLiteral");
+
----------------
rsmith wrote:
> There are lots of forms of expression that cannot be the base of an `LValue`
> (see the list above `LValueExprEvaluator` for the expression forms that *can*
> be the base of an `LValue`); is it important to give this one special
> treatment?
Because a `SourceLocExpr` *can* be the base of an `LValue`. But the way that's
supported is by transforming the `SourceLocExpr` into a `StringLiteral`.
================
Comment at: lib/CodeGen/CGExprConstant.cpp:1762
+ConstantLValue
+ConstantLValueEmitter::VisitSourceLocExpr(const SourceLocExpr *E) {
+ const APValue::LValueBase& Base = Value.getLValueBase();
----------------
rsmith wrote:
> Hmm, is this reachable? I thought `SourceLocExpr` was always a prvalue.
You're right. Not sure what I was thinking.
================
Comment at: lib/Sema/TreeTransform.h:9990
+ bool NeedRebuildFunc = E->getIdentType() == SourceLocExpr::Function &&
+ isa<FunctionDecl>(getSema().CurContext) &&
+ getSema().CurContext != E->getParentContext();
----------------
rsmith wrote:
> If we generalize `__builtin_FUNCTION()` to behave like `__FUNCTION__` (and
> handle function-like contexts that aren't `FunctionDecl`s), this `isa` check
> will be wrong.
I'll remove it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D37035/new/
https://reviews.llvm.org/D37035
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
