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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 13 inline comments as done.
EricWF added a comment.

Address review comments. Updated patch coming shortly.



================
Comment at: include/clang/AST/Expr.h:4147
+  SourceLocation getBeginLoc() const LLVM_READONLY { return BuiltinLoc; }
+  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }
+
----------------
riccibruno wrote:
> 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 ?
It probably doesn't make any difference. Removing.


================
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();
----------------
riccibruno wrote:
> 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...)
It should be. I'll fix it to act like `ActOnStringLiteral` and then deduplicate the code.


================
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();
----------------
riccibruno wrote:
> Is it taking into account `adjustStringLiteralBaseType` as in `getStringLiteralArrayType` ?
Nope. Fixed.


================
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())
----------------
riccibruno wrote:
> This comment is strange since MkStr will call `getPredefinedStringLiteralFromCache`
> when `FD->getDeclName()` is a simple identifier.
I think this should say `compute` instead of `cache`.


================
Comment at: lib/AST/Expr.cpp:2037
+    llvm::APSInt IntVal(
+        llvm::APInt(Ctx.getTargetInfo().getIntWidth(), LineOrCol));
+    return APValue(IntVal);
----------------
riccibruno wrote:
> Both `getLine`, `getColumn` and `APInt` take an unsigned.
What's your objection here? That I used `int64_t` or `getIntWidth()`? I've reworked this bit of code. Let me know if it's still problematic.


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

https://reviews.llvm.org/D37035