cor3ntin added a comment.

This generally looks good to me but i have a couple remarks



================
Comment at: clang/lib/Frontend/TextDiagnostic.cpp:124-128
+  if (CharSize == 1 && llvm::isLegalUTF8Sequence(Begin, End) &&
+      llvm::sys::locale::isPrint(*Begin)) {
+    ++(*I);
+    return std::make_pair(SmallString<16>(Begin, End), true);
+  }
----------------
this could be simplified : the common case for ascii could be just looking at 
`isPrint(*Begin);` (which implies  CharSize == 1 and  
llvm::isLegalUTF8Sequence(Begin, End))
So you could do it before computing CharSize


================
Comment at: clang/lib/Frontend/TextDiagnostic.cpp:132
+  // Convert it to UTF32 and check if it's printable.
+  if (llvm::isLegalUTF8Sequence(Begin, End)) {
+    llvm::UTF32 C;
----------------
I think we should check that `CharSize` is not greater than `Begin + 
SourceLine.size() - *i`  or something along those lines otherwise we may 
overflow SourceLine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150843

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to