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

Seth Cantrell seth.cantrell at gmail.com
Sun Apr 15 16:56:12 PDT 2012

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:

-------------- 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: 36791 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120415/96db706d/attachment.obj>

More information about the cfe-dev mailing list