I made these changes because I found it easier to think about if NewStart and NewEnd always maintained `map.byteToColumn() != -1`. I don't have any particular test case and I actually don't see any path through the code that wouldn't call startOfPreviousColumn()/startOfNextColumn() later anyway, except if SourceStart-1 / SourceEnd+1 ends up pointing to the beginning of the line / past the end of the line, and there shouldn't be any way for byteToColumn() to be -1 at those locations.
I agree that we should not be using any locale sensitive functions here (or anywhere except maybe a few places where we might care about the locale). I believe we expect to run in the "C" locale, in which case isspace() may only return true for the standard whitespace characters. That works fine on ASCII compatible systems, but would cause problems on EBCDIC or otherwise non-ASCII-compatible systems. There are also other places we'd have problems with non-ASCII-compatible systems, such as uses of character literals which wouldn't have the expected ASCII values. On Nov 5, 2012, at 2:46 PM, Richard Smith <[email protected]> wrote: > Looks like this would only make a difference if isspace() returns true > for some UTF-8 trailing byte in the current locale. However, the code > is still broken in locales in which isspace() returns true for some > UTF-8 leading bytes. We should not be using isspace() here... > > On Mon, Nov 5, 2012 at 6:34 AM, Rafael Espíndola > <[email protected]> wrote: >> testcase? >> >> On 3 November 2012 17:21, Seth Cantrell <[email protected]> wrote: >>> Author: socantre >>> Date: Sat Nov 3 16:21:17 2012 >>> New Revision: 167361 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=167361&view=rev >>> Log: >>> don't step into the middle of multibyte sequences >>> >>> Modified: >>> cfe/trunk/lib/Frontend/TextDiagnostic.cpp >>> >>> Modified: cfe/trunk/lib/Frontend/TextDiagnostic.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/TextDiagnostic.cpp?rev=167361&r1=167360&r2=167361&view=diff >>> ============================================================================== >>> --- cfe/trunk/lib/Frontend/TextDiagnostic.cpp (original) >>> +++ cfe/trunk/lib/Frontend/TextDiagnostic.cpp Sat Nov 3 16:21:17 2012 >>> @@ -418,7 +418,7 @@ >>> bool ExpandedRegion = false; >>> >>> if (SourceStart>0) { >>> - unsigned NewStart = SourceStart-1; >>> + unsigned NewStart = map.startOfPreviousColumn(SourceStart); >>> >>> // Skip over any whitespace we see here; we're looking for >>> // another bit of interesting text. >>> @@ -445,7 +445,7 @@ >>> } >>> >>> if (SourceEnd<SourceLine.size()) { >>> - unsigned NewEnd = SourceEnd+1; >>> + unsigned NewEnd = map.startOfNextColumn(SourceEnd); >>> >>> // Skip over any whitespace we see here; we're looking for >>> // another bit of interesting text. >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
