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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.


================
Comment at: include/clang/AST/Expr.h:4108
+public:
+  enum IdentType { Function, File, Line, Column };
+
----------------
`IdentType` -> `IdentKind` ?


================
Comment at: include/clang/AST/Expr.h:4147
+  SourceLocation getBeginLoc() const LLVM_READONLY { return BuiltinLoc; }
+  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }
+
----------------
I don't think that `LLVM_READONLY` is useful here since it is just a simple getter.
Same remark for `getIdentType`, `isStringType` and `isIntType`.
And even for `getBuiltinStr` does it actually make a difference ?


================
Comment at: include/clang/AST/Stmt.h:657
 
+  class SourceLocExprBitfields {
+    friend class SourceLocExpr;
----------------
Could you please put this in the same place as in
`StmtNodes.td` ? (ie just after `PseudoObjectExpr`)
At least not under "C++ coroutines TS bitfields classes"...


================
Comment at: lib/AST/ASTContext.cpp:10145
+                                               unsigned Length) const {
+  // A C++ string literal has a const-qualified element type (C++ 2.13.4p1).
+  EltTy = EltTy.withConst();
----------------
And what about C ? It seems to me that `getStringLiteralArrayType`
should be used systematically when the type of a string literal is needed.

(eg in `ActOnStringLiteral` but this is not the only place...)


================
Comment at: lib/AST/Expr.cpp:1961
+    // A C++ string literal has a const-qualified element type (C++ 2.13.4p1).
+    if (Ctx.getLangOpts().CPlusPlus || Ctx.getLangOpts().ConstStrings)
+      Ty = Ty.withConst();
----------------
Is it taking into account `adjustStringLiteralBaseType` as in `getStringLiteralArrayType` ?


================
Comment at: lib/AST/Expr.cpp:1978
+      BuiltinLoc(BLoc), RParenLoc(RParenLoc), ParentContext(ParentContext) {
+  SourceLocExprBits.Type = Type;
+}
----------------
The name `Type` here is really unfortunate imho.


================
Comment at: lib/AST/Expr.cpp:1992
+  }
+}
+
----------------
`llvm_unreachable("unexpected IdentType!")`


================
Comment at: lib/AST/Expr.cpp:2025
+      // If we have a simple identifier there is no need to cache the
+      // human readable name.
+      if (IdentifierInfo *II = FD->getDeclName().getAsIdentifierInfo())
----------------
This comment is strange since MkStr will call `getPredefinedStringLiteralFromCache`
when `FD->getDeclName()` is a simple identifier.


================
Comment at: lib/AST/Expr.cpp:2037
+    llvm::APSInt IntVal(
+        llvm::APInt(Ctx.getTargetInfo().getIntWidth(), LineOrCol));
+    return APValue(IntVal);
----------------
Both `getLine`, `getColumn` and `APInt` take an unsigned.


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

https://reviews.llvm.org/D37035