<html><head></head><body bgcolor="#FFFFFF"><div>These are addressed in the committed version.</div><div><br></div><div>llvm: r154944/fdc97cd2</div><div>clang: r154947/70712b27</div><div><br>On Apr 17, 2012, at 1:48 PM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>> wrote:<br>
<br></div><div></div><blockquote type="cite"><div><div>More nits for the llvm patch:</div><div><br></div><div>-Use lower case for the first letter of functions, as dictated by llvm conventions.</div><div>-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.</div>
<div>-For managing the 'locale_holder' object, use llvm::ManagedStatic, which handles lifetime of such objects and allows them to be created lazily when needed.</div><div><br></div><div>-Argyrios</div><br><div><div>
On Apr 16, 2012, at 6:49 PM, Seth Cantrell wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap:break-word"><div><div>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.</div>
<div><br></div><div>let me know if these are okay to commit.</div><div><br></div><div>- Thanks</div></div><div><br></div><div><br></div><br><div><div>On Apr 16, 2012, at 9:46 PM, Craig Topper wrote:</div><br class="Apple-interchange-newline">
<blockquote type="cite"><div style="text-align:left">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.<br>

<br>~Craig<br></div><br><div class="gmail_quote">On Mon, Apr 16, 2012 at 12:43 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

Would it be possible to move the platform specific stuff down to llvm ?<br>
<br>
Also, instead of passing "SmallVector<int,200>"'s around could you create a class to encapsulate that functionality ?<br>
<br>
-Argyrios<br>
<br>
On Apr 15, 2012, at 7:38 PM, Seth Cantrell wrote:<br>
<br>
> iswprint does turn out to return different results; it's saying all non-ascii characters are unprintable.<br>
><br>
> 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.<br>


><br>
> Here's the patch with that done:<br>
><br>
> <0001-Nicer-display-of-unprintable-source-and-fix-caret-di.patch><br>
><br>
> 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.<br>
><br>
> - Seth<br>
><br>
><br>
> On Apr 15, 2012, at 7:56 PM, Seth Cantrell wrote:<br>
><br>
>><br>
>> On Apr 15, 2012, at 5:00 PM, Benjamin Kramer wrote:<br>
>><br>
>>><br>
>>> On 15.04.2012, at 21:56, Seth Cantrell wrote:<br>
>>><br>
>>>> 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.<br>


>>>><br>
>>>> 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`<br>


>>><br>
>>> The 2 small patches look good.<br>
>>><br>
>>> 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?<br>


>><br>
>> 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.<br>


>><br>
>> 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<br>


>><br>
>><br>
>>><br>
>>> Apart from that there are some stylistic issues:<br>
>>> - 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.<br>


>>> - Replace 'const std::string &' with StringRef<br>
>>> - The preferred style is to put const in front of the type: "const int*" instead of "int const*"<br>
>>> - Opening braces always go on the same line as the declaration.<br>
>>> - Be consistent about spacing, "if (" instead of "if(", always put spaces after commas, …<br>
>>><br>
>>> - Ben<br>
>><br>
>><br>
>> Okay, this should address the small issues:<br>
>><br>
>> <0001-Nicer-display-of-unprintable-source-and-fix-caret-di.patch><br>
><br>
> _______________________________________________<br>
> cfe-dev mailing list<br>
> <a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br>
<br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
</blockquote></div><br><br clear="all"><br>-- <br>~Craig<br>
</blockquote></div></div><span><0001-platform-support-for-counting-column-widths-and-chec.patch></span><div style="word-wrap:break-word"></div><span><0001-Nicer-display-of-unprintable-source-and-fix-caret-di.patch></span><div style="word-wrap:break-word">
</div><span><0002-fix-display-of-source-lines-with-null-characters.patch></span><div style="word-wrap:break-word"></div></blockquote></div><br></div></blockquote></body></html>