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

[Eric Fiselier via Phabricator via cfe-commits]

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