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

Seth Cantrell seth.cantrell at gmail.com
Tue Apr 17 13:22:58 PDT 2012

These are addressed in the committed version.

llvm: r154944/fdc97cd2
clang: r154947/70712b27

On Apr 17, 2012, at 1:48 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:

More nits for the llvm patch:

-Use lower case for the first letter of functions, as dictated by llvm
-For columnWidth accept a StringRef as parameter which is more versatile,
and copy it inside the function to get a null-terminated c string. You are
already copying the string anyway.
-For managing the 'locale_holder' object, use llvm::ManagedStatic, which
handles lifetime of such objects and allows them to be created lazily when


On Apr 16, 2012, at 6:49 PM, Seth Cantrell wrote:

Okay, here are patches for review with those changes. Built and 'make
check'ed and 'make test'ed on OS X. Still building on Windows.

let me know if these are okay to commit.

- Thanks

On Apr 16, 2012, at 9:46 PM, Craig Topper wrote:

For function arguments it should use SmallVectorImpl<int> instead of
SmallVector<int, 200>. Its probably bad to return a SmallVector since you'd
incur a copy on the return. If possible it would probably be better to pass
in a SmallVector to be modified instead.


On Mon, Apr 16, 2012 at 12:43 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com>wrote:

> Would it be possible to move the platform specific stuff down to llvm ?
> Also, instead of passing "SmallVector<int,200>"'s around could you create
> a class to encapsulate that functionality ?
> -Argyrios
> On Apr 15, 2012, at 7:38 PM, Seth Cantrell wrote:
> > iswprint does turn out to return different results; it's saying all
> non-ascii characters are unprintable.
> >
> > I can implement a generic version of this stuff for platforms that don't
> have xlocale. In fact, since we don't yet have support for printing out
> non-ascii text on Windows I can replace my current Windows implementation
> with the generic implementation.
> >
> > Here's the patch with that done:
> >
> > <0001-Nicer-display-of-unprintable-source-and-fix-caret-di.patch>
> >
> > Currently the xlocale version is restricted based on ifdef __APPLE__,
> while everything else gets the generic version. Maybe somebody else knows
> of a better flag to use.
> >
> > - Seth
> >
> >
> > On Apr 15, 2012, at 7:56 PM, Seth Cantrell wrote:
> >
> >>
> >> On Apr 15, 2012, at 5:00 PM, Benjamin Kramer wrote:
> >>
> >>>
> >>> On 15.04.2012, at 21:56, Seth Cantrell wrote:
> >>>
> >>>> I'm still hopeful that this change can make it in in time for the 3.1
> branch tomorrow. If anyone happens to see this who can review these patches
> before the branch I'd be grateful. Hopefully there are no major issues so
> it will be able to go in.
> >>>>
> >>>> The first patch is just to add support for reversed colors in
> raw_ostreams in llvm. The second patch uses it to print out unprintable
> characters, and also adjusts text diagnostic ranges and fix-its for both
> the escaped representations and for characters that aren't a single column
> wide. The last patch is unimportant and for another issue: printing
> diagnostics for source that contains `\0`
> >>>
> >>> The 2 small patches look good.
> >>>
> >>> The biggest issue I have with the big patch is that the foo_l
> functions are non-portable. They don't exist on linux and other non-BSD
> unices. I find it worrying to have locale stuff in clang, is there really
> no other way to get this info?
> >>
> >> wcswidth* is the only api I know of for computing the terminal display
> column width of text. I think I could switch the iswprint_l and wcswidth_l
> functions for non-_l versions. iswprint and wcswidth technically use the
> current locale but I don't think the locale will actually change the
> results. I've seen isprint used elsewhere so I guess functions based on the
> current locale would be okay.
> >>
> >> But I do have to convert from UTF-8 to the host's wchar_t encoding in
> order to use these functions. Currently I'm creating a locale where the
> narrow character encoding is UTF-8 so I can convert from UTF-8 to the
> wchar_t encoding. And since I go ahead and use that for the other locale
> based functions. I could assume that the wchar_t is UTF-16 or UTF-32
> depending on the sizeof(wchar_t), and use the ConvertUTF8ToUTFXX functions.
> As far as I know this holds for the platforms we care about. Another
> alternative would be the
> >>
> >>
> >>>
> >>> Apart from that there are some stylistic issues:
> >>> - Don't pass around pairs of char*, use StringRef instead. You can
> also replace some loops with its convenience functions, e.g.
> StringRef::rfind. The stuff from ConvertUTF.h is an exception as it's
> contributed code, but its good to do the conversion to from StringRef to
> char* as late as possible to keep typecasting at a minimum.
> >>> - Replace 'const std::string &' with StringRef
> >>> - The preferred style is to put const in front of the type: "const
> int*" instead of "int const*"
> >>> - Opening braces always go on the same line as the declaration.
> >>> - Be consistent about spacing, "if (" instead of "if(", always put
> spaces after commas, …
> >>>
> >>> - Ben
> >>
> >>
> >> Okay, this should address the small issues:
> >>
> >> <0001-Nicer-display-of-unprintable-source-and-fix-caret-di.patch>
> >
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120417/6468551e/attachment.html>

More information about the cfe-dev mailing list