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

[Craig Topper]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120416/a109419c/attachment.html>