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

[Benjamin Kramer]

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?

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