[cfe-commits] r167361 - /cfe/trunk/lib/Frontend/TextDiagnostic.cpp

[Seth Cantrell]

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 <richard at metafoo.co.uk> 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
> <rafael.espindola at gmail.com> wrote:
>> testcase?
>> 
>> On 3 November 2012 17:21, Seth Cantrell <seth.cantrell at gmail.com> 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
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits