rsmith added inline comments.
================
Comment at: include/clang/AST/ASTContext.h:279
+ /// A cache mapping a function declaration to its human-readable function or
+ /// file name.
----------------
Comment seems out of date: the key here is a string rather than a function
declaration.
================
Comment at: lib/AST/Expr.cpp:2012-2020
+ case SourceLocExpr::Function:
+ case SourceLocExpr::File: {
+ return [&]() {
+ auto MkStr = [&](StringRef Tmp) {
+ StringLiteral *Res = Ctx.getPredefinedStringLiteralFromCache(Tmp);
+ return APValue(Res, CharUnits::Zero(), APValue::NoLValuePath(), 0);
+ };
----------------
Combining these two cases doesn't really make sense since they have nothing in
common (except that both produce strings). Instead, consider hoisting the
`MkStr` lambda out of the switch and separating the cases.
================
Comment at: lib/AST/Expr.cpp:2022-2023
+ const auto *FD = dyn_cast_or_null<FunctionDecl>(Context);
+ if (!FD)
+ return MkStr("");
+ // If we have a simple identifier there is no need to cache the
----------------
Why not call into `PredefinedExpr` for the other cases that `__FUNCTION__`
supports (`BlockDecl`, `CapturedDecl`, `ObjCMethodDecl`)?
================
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));
----------------
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).
================
Comment at: lib/AST/ExprConstant.cpp:2653-2655
+ APValue::LValueBase const &Base,
uint64_t Index) {
+ const Expr *Lit = Base.get<const Expr *>();
----------------
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?
================
Comment at: lib/AST/ExprConstant.cpp:3370-3371
+
+ assert((!Base || !isa<SourceLocExpr>(Base)) &&
+ "Base should have already been transformed into a StringLiteral");
+
----------------
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?
================
Comment at: lib/CodeGen/CGExprConstant.cpp:1762
+ConstantLValue
+ConstantLValueEmitter::VisitSourceLocExpr(const SourceLocExpr *E) {
+ const APValue::LValueBase& Base = Value.getLValueBase();
----------------
Hmm, is this reachable? I thought `SourceLocExpr` was always a prvalue.
================
Comment at: lib/CodeGen/CGExprConstant.cpp:1766
+ "Expected string literal result");
+ const StringLiteral* Str = cast<StringLiteral>(Base.get<const Expr*>());
+
----------------
`const auto *Str`
================
Comment at: lib/CodeGen/CGExprConstant.cpp:1768-1769
+
+ // assert(Base.isGlobalString() &&
+ // "source location expression does not evaluate to global string?");
+ return CGM.GetAddrOfConstantCString(Str->getBytes());
----------------
Please remove or uncomment this.
================
Comment at: lib/Sema/TreeTransform.h:9990
+ bool NeedRebuildFunc = E->getIdentType() == SourceLocExpr::Function &&
+ isa<FunctionDecl>(getSema().CurContext) &&
+ getSema().CurContext != E->getParentContext();
----------------
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.
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
