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

[Seth Cantrell]

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.


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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Nicer-display-of-unprintable-source-and-fix-caret-di.patch
Type: application/octet-stream
Size: 32965 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120417/6034a920/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-fix-display-of-source-lines-with-null-characters.patch
Type: application/octet-stream
Size: 2021 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120417/6034a920/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-tests-for-diagnostics-on-unprintable-source.patch
Type: application/octet-stream
Size: 1681 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120417/6034a920/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-platform-support-for-counting-column-widths-and-chec.patch
Type: application/octet-stream
Size: 4203 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120417/6034a920/attachment-0003.obj>