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); + } ---------------- tbaeder wrote: > 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. Oh also, there are two asserts at the beginning of the function to ensure `I` is valid. 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