[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
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);
+  }
----------------
cor3ntin wrote:
> tbaeder wrote:
> > cor3ntin wrote:
> > > 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
> > This is not true in my testing fwiw.
> ```
> const unsigned char *Begin = SourceLine.bytes_begin() + *I;
> 
>   // Fast path for the common ASCII case.
>   if (*Begin < 0x80 && llvm::sys::locale::isPrint(*Begin)) {
>     ++(*I);
>     return std::make_pair(SmallString<16>(Begin, Begin + 1), true);
>   }
> ```
> seems to work fine locally. Note that I'm not sure `*Begin` is always valid - it seems to be, but we might want an assert to check that SourceLine is not empty. 
This function is only ever called in a `while (I < SourceLine.size())` loop. I've thought about refactoring this into a helper struct that keeps the index separate from the calling function to simplify callers.


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

https://reviews.llvm.org/D150843