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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
