cor3ntin added a comment.
Give some time for @tahonermann to have an opportunity to review again, but
otherwise this looks good to me. Thanks!
================
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:
> 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.
you are right, if, `SourceLine` is empty, that first asset would always fire
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
