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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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