[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/SourceLocExprScope.h:39
+    //    static_assert(foo() == __LINE__);
+    return OldVal == nullptr || isa<CXXDefaultInitExpr>(DefaultExpr) ||
+           !OldVal->isInSameContext(EvalContext);
----------------
Do we want the `isa` check here? I'd think the same problem would occur for default initializers:

```
struct A {
  int n = __builtin_LINE();
};
struct B {
  A a = {};
};
B b = {}; // should give this context as the current one, not the context of the initializer inside struct B
```


================
Comment at: include/clang/AST/SourceLocExprScope.h:44
+public:
+  SourceLocExprScopeBase(const Expr *DefaultExpr, SourceLocExprScopeBase **Dest,
+                         EvalContextType *EvalContext)
----------------
I think it'd be cleaner to add a

```
struct Current {
  SourceLocExprScopeBase *Scope;
};
```

maybe with members to get the location and context for a given `SourceLocExpr`, and to pass in a `SourceLocExprScopeBase::Current &` here rather than a `SourceLocExprScopeBase **`.


================
Comment at: include/clang/Sema/Sema.h:4463
+
+  /// \brief build a potentially resolved SourceLocExpr.
+  ///
----------------
Capitalize "build" here.


================
Comment at: include/clang/Serialization/ASTBitCodes.h:1643
 
+      /// A SourceLocExpr record
+      EXPR_SOURCE_LOC,
----------------
Missing period.


================
Comment at: lib/AST/Expr.cpp:1924-1931
+  auto CreateString = [&](StringRef SVal) {
+    QualType StrTy = BuildSourceLocExprType(Ctx, getIdentType(),
+                                            /*IsArray*/ true, SVal.size() + 1);
+    StringLiteral *Lit = StringLiteral::Create(Ctx, SVal, StringLiteral::Ascii,
+                                               /*Pascal*/ false, StrTy, Loc);
+    assert(Lit && "should not be null");
+    return Lit;
----------------
It doesn't seem reasonable to create a new `StringLiteral` each time a `SourceLocExpr` is (constant-)evaluated, and seems like it would be unsound in some cases (we require that reevaluation produces the same result).

I think the `StringLiteral` here is just for convenience, right? (Instead we could model the `SourceLocExpr` as being its own kind of array lvalue for the purpose of constant expression evaluation, like we do for `ObjCEncodeExpr` for instance.)


================
Comment at: lib/AST/ExprConstant.cpp:698-704
+    /// \brief Source location information about the default argument expression
+    /// we're evaluating, if any.
+    SourceLocExprScope *CurCXXDefaultArgScope = nullptr;
+
+    /// \brief Source location information about the default member initializer
+    /// we're evaluating, if any.
+    SourceLocExprScope *CurCXXDefaultInitScope = nullptr;
----------------
Do we really need both of these? It would seem like one 'current' scope would be sufficient to me.


================
Comment at: lib/AST/ExprConstant.cpp:7296
+bool IntExprEvaluator::VisitSourceLocExpr(const SourceLocExpr *E) {
+    assert(E && E->isLineOrColumn());
+    llvm::APInt Result;
----------------
Too much indentation.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:192-199
+    void VisitVAArgExpr(VAArgExpr * E);
 
-  void EmitInitializationToLValue(Expr *E, LValue Address);
-  void EmitNullInitializationToLValue(LValue Address);
-  //  case Expr::ChooseExprClass:
-  void VisitCXXThrowExpr(const CXXThrowExpr *E) { CGF.EmitCXXThrowExpr(E); }
-  void VisitAtomicExpr(AtomicExpr *E) {
-    RValue Res = CGF.EmitAtomicExpr(E);
-    EmitFinalDestCopy(E->getType(), Res);
-  }
+    void EmitInitializationToLValue(Expr * E, LValue Address);
+    void EmitNullInitializationToLValue(LValue Address);
+    //  case Expr::ChooseExprClass:
+    void VisitCXXThrowExpr(const CXXThrowExpr *E) { CGF.EmitCXXThrowExpr(E); }
+    void VisitAtomicExpr(AtomicExpr * E) {
----------------
Something funny happened to the formatting here.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:595
+    } else {
+      StringLiteral *Str = SLE->getStringValue(Ctx, Loc, DC);
+      return CGF.EmitArrayToPointerDecay(Str).getPointer();
----------------
Should we make some attempt to reuse the same string literal for multiple source locations denoting the same file?


https://reviews.llvm.org/D37035