[cfe-dev] [PATCH] pretty display of unprintable source and correct caret/range/fixup alignment

[Seth Cantrell]

Okay, I'll commit to trunk in a bit then.

Thanks,
Seth

On Apr 17, 2012, at 1:04 PM, Douglas Gregor <dgregor at apple.com> wrote:

> Hi Seth,
>
> On Apr 17, 2012, at 9:51 AM, Seth Cantrell <seth.cantrell at gmail.com> wrote:
>
>> I have a patch I think should go in 3.1: A change in the clang
>> FrontEnd text diagnostics to display 'unprintable' source such as
>> source with a wrong encoding or characters that can't be displayed in
>> the terminal. Additionally range highlighting, fixups and caret
>> locations are corrected both for characters that take multiple columns
>> (e.g., wide east asian characters) and for the displayed 'unprintable'
>> characters.
>>
>> This patch to the frontend in clang relies on an addition to llvm that
>> supports platform specific column width calculation and is_print
>> checks. (The area this code was added too appears 'unowned' on the
>> list of source code owners)
>>
>> These changes to text diagnostics display are complimentary to the
>> improved support for unicode source added since clang 3.0.
>>
>> Patches are attached, and emails from reviewers that suggested changes
>> (which have been applied) are quoted below.
>
> Thank you for working on this. I think it's a great fix for top-of-tree Clang. However, since we're not talking about fixing a regression and the vast majority of users are unlikely to ever run into this problem, I do not think we should take this patch for 3.1.
>
>    - Doug
>
>> On Mon, Apr 16, 2012 at 3:43 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>> Would it be possible to move the platform specific stuff down to llvm ?
>>
>> On Apr 16, 2012, at 5:06 PM, Seth Cantrell wrote:
>>> It doesn't look like there's currently anything platform specific in the llvm/ADT lib. How about llvm/Support?
>>>
>>> include/llvm/Support/TextDiagnosticPlatformSupport.h
>>> lib/Support/TextDiagnosticGenericSupport.inc
>>> lib/Support/TextDiagnosticPlatformSupport.cpp
>>> lib/Support/TextDiagnosticWindowsSupport.inc
>>> lib/Support/TextDiagnosticXlocaleSupport.inc
>>
>>> Argyrios Kyrtzidis kyrtzidis at apple.com
>>> 8:13 PM (15 hours ago)
>>>
>>> Could we drop 'TextDiagnostic' since we are talking about 2 locale-specific functions ?
>>>
>>> How about
>>>
>>> include/llvm/Support/Locale.h
>>> lib/Support/Locale.cpp
>>> [...]
>>>
>>>
>>> And put the functions in Locale.h in a 'locale' namespace, similar to llvm::sys::path :
>>>
>>> namespace llvm {
>>> namespace sys {
>>> namespace locale {
>>> [...]
>>>
>>
>>
>> On Apr 16, 2012, at 11:14 PM, Craig Topper wrote:
>>> byteToColumn and columnToByte bodies probably shouldn't be buried inside
>>> SourceColumnMap. The indentation is confusing because of it.
>>>
>>> Believe the general still is to keep the operator at the end of the previous
>>> line when splitting lines
>>>
>>> +  assert(CaretStart!=(unsigned)-1 && CaretEnd!=(unsigned)-1
>>> +         && SourceStart!=(unsigned)-1 && SourceEnd!=(unsigned)-1);
>>>
>>> +    std::pair<SmallString<16>,bool> res
>>> +        = printableTextForNextCharacter(line, &i, DiagOpts.TabStop);
>>>
>>>
>>> This is a nit, but the 'else' isn't needed since the 'if' ends in a return.
>>>
>>> +      return std::make_pair(expandedCP, false);
>>> +    } else {
>>> +      // If next character is valid UTF-8, and printable
>>> +      return std::make_pair(SmallString<16>(original_begin, cp_end), true);
>>> +    }
>>>
>>> ~Craig
>>>
>> <0001-Nicer-display-of-unprintable-source-and-fix-caret-di.patch><0002-fix-display-of-source-lines-with-null-characters.patch><0003-Add-tests-for-diagnostics-on-unprintable-source.patch><0001-platform-support-for-counting-column-widths-and-chec.patch>
>