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

[Douglas Gregor]

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>