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

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

Some cleanups and simplifications, but LGTM with those applied. Thank you!



================
Comment at: docs/LanguageExtensions.rst:2335
+
+When the builtins appears as part of a default function argument the invocation
+point is the location of the caller. When the builtins appear as part of a
----------------
appears -> appear


================
Comment at: include/clang/AST/CurrentSourceLocExprScope.h:31
+public:
+  /// A RAII style scope gaurd used for tracking the current source
+  /// location and context as used by the source location builtins
----------------
gaurd -> guard


================
Comment at: include/clang/AST/Expr.h:4168
+/// Represents a function call to one of __builtin_LINE(), __builtin_COLUMN(),
+/// __builtin_FUNCTION(), or __builtin_FILE()
+class SourceLocExpr final : public Expr {
----------------
Missing period at end of comment.


================
Comment at: lib/AST/ASTContext.cpp:10151-10153
+  // Get an array type for the string, according to C99 6.4.5.  This includes
+  // the nul terminator character as well as the string length for pascal
+  // strings.
----------------
"the string length for pascal strings" part here seems out of place, since you don't handle that here.


================
Comment at: lib/AST/Expr.cpp:2073
+    StringLiteral *Res = Ctx.getPredefinedStringLiteralFromCache(Tmp);
+    return APValue(Res, CharUnits::Zero(), APValue::NoLValuePath(), 0);
+  };
----------------
Given that the type of the `SourceLocExpr` is `const char*`, I'd expect the `APValue` result to be a decayed pointer to the first character in the string, not a pointer to the string itself. (So I think this should have a path of length 1 containing `LValuePathEntry{.ArrayIndex = 0}`.)


================
Comment at: lib/AST/ExprConstant.cpp:5861-5863
+    Result.addArray(
+        Info, E,
+        cast<ConstantArrayType>(Str->getType()->getAsArrayTypeUnsafe()));
----------------
(This won't be necessary if/when `EvaluateInContext` returns a pointer to the first character in the string.)


================
Comment at: lib/AST/ExprConstant.cpp:7616
+      Info.Ctx, Info.CurrentCall->CurSourceLocExprScope.getDefaultExpr());
+  return Success(Evaluated.getInt(), E);
+}
----------------
Consider dropping the `.getInt()` here? (Then this version and the pointer version will be identical, and we could consider moving this to `ExprEvaluatorBase`.)


================
Comment at: lib/AST/ExprConstant.cpp:11286
+  case Expr::SourceLocExprClass:
     // GCC considers the GNU __null value to be an integral constant expression.
     return NoDiag();
----------------
This comment is getting detached from its case label. Maybe just remove it? It seems reasonably obvious that `__null` should be an ICE.


================
Comment at: lib/AST/ExprConstant.cpp:3370-3371
+
+  assert((!Base || !isa<SourceLocExpr>(Base)) &&
+         "Base should have already been transformed into a StringLiteral");
+
----------------
EricWF wrote:
> 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`. 
I don't agree: a `SourceLocExpr` cannot be the base of an `LValue`. It is evaluated into something else that can be (a `StringLiteral`), but it itself cannot be (and this is in no way unusual; that's probably true of most `Expr` nodes). I think this is a remnant of an earlier design?


================
Comment at: lib/CodeGen/CGExpr.cpp:3241-3281
 Address CodeGenFunction::EmitArrayToPointerDecay(const Expr *E,
                                                  LValueBaseInfo *BaseInfo,
                                                  TBAAAccessInfo *TBAAInfo) {
-  assert(E->getType()->isArrayType() &&
+  LValue LV = EmitLValue(E);
+  return EmitArrayToPointerDecay(E, E->getType(), LV, BaseInfo, TBAAInfo);
+}
+
----------------
This change should be unnecessary after changing `EvaluateInContext` to decay the pointer.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:623-634
+
+    if (SLE->isIntType())
+      return Builder.getInt(Evaluated.getInt());
+
+    // If we're not building an int, then we're building a string literal.
+    const APValue::LValueBase &Base = Evaluated.getLValueBase();
+    const StringLiteral *Str = cast<StringLiteral>(Base.get<const Expr *>());
----------------
This can be simplified to:
```
    return ConstantEmitter(CGF.CGM, CGF).emitAbstract(SLE->getLocation(), Evaluated, SLE->getType());
```
(once `EvaluateInContext` decays the pointer).


================
Comment at: lib/Serialization/ASTReaderStmt.cpp:971-977
+void ASTStmtReader::VisitSourceLocExpr(SourceLocExpr *E) {
+  VisitExpr(E);
+  E->setParentContext(ReadDeclAs<DeclContext>());
+  E->setBeginLoc(ReadSourceLocation());
+  E->setEndLoc(ReadSourceLocation());
+  E->setIdentKind(static_cast<SourceLocExpr::IdentKind>(Record.readInt()));
+}
----------------
Please directly set the fields here and remove the setters; we generally don't want AST nodes to have setter functions, as they're supposed to be immutable after creation. (Deserialization is an exception to this only because it is creating the nodes itself.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37035/new/

https://reviews.llvm.org/D37035