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

[Craig Topper]

This style is kind of hard to read as it makes arguments look like local
variables at first glance. If it's a line length problem, putting "static
void" on a separate line is probably a better choice so that the argument
can still line up with the the parenthese.

+static void selectInterestingSourceRegion(
+  std::string &SourceLine,
+  std::string &CaretLine,
+  std::string &FixItInsertionLine,
+  unsigned Columns,
+  const SourceColumnMap &map) {

On Mon, Apr 16, 2012 at 6:49 PM, Seth Cantrell <seth.cantrell at gmail.com>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.
>
> ~Craig
>
> 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
>>
>
>
>
> --
> ~Craig
>
>
>
>
>
>


-- 
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120416/54b905d8/attachment.html>