johannes requested changes to this revision.
johannes added a comment.
This revision now requires changes to proceed.
I've suggested some more refactorings but otherwise this should be good to go
================
Comment at: clang/lib/Tooling/ASTDiff/ASTDiff.cpp:497
+ return str;
+ }
return std::string(String->getString());
----------------
Sorry for the delay. Looks pretty good already.
think there's a bit of redundancy now, I tried to simplify it, WDYT?
I used uppercase variable names as this is the local convention. I'm not sure
what's the global one.
`String->getByteLength() / String->getCharByteWidth();` adds an unnecessary
division, we can just use getLength().
```
if (String->isWide() || String->isUTF16() || String->isUTF32()) {
std::string UTF8Str;
unsigned int NumChars = String->getLength();
const char *Bytes = String->getBytes().data();
if (String->isWide()) {
const auto *Chars = reinterpret_cast<const wchar_t *>(Bytes);
if (!convertWideToUTF8({Chars, NumChars}, UTF8Str))
return "";
} else if (String->isUTF16()) {
const auto *Chars = reinterpret_cast<const unsigned short *>(Bytes);
if (!convertUTF16ToUTF8String({Chars, NumChars}, UTF8Str))
return "";
} else {
assert(String->isUTF32() && "Unsupported string encoding.");
const auto *Chars = reinterpret_cast<const unsigned int *>(Bytes);
if (!convertUTF32ToUTF8String({Chars, NumChars}, UTF8Str))
return "";
}
return UTF8Str;
}
```
================
Comment at: clang/test/Tooling/clang-diff-ast.cpp:74
+ return U"foo";
+ // CHECK-NOT: ImplicitCastExpr
+ return 0;
----------------
I think two lines per encoding should be enough, so how about doing this right
next to the existing string literal?
```
const char *foo(int i) {
if (i == 0)
// CHECK: StringLiteral: foo(
return "foo";
// CHECK: StringLiteral: wide(
(void)L"wide";
// CHECK: StringLiteral: utf-16(
(void)u"utf-16";
// CHECK: StringLiteral: utf-32(
(void)U"utf-32";
// CHECK-NOT: ImplicitCastExpr
return 0;
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126651/new/
https://reviews.llvm.org/D126651
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
