<p dir="ltr">Do you have a version with the last suggestions? I lgtmed it conditional on it.   Do you need someone to commit it for you? </p>
<div class="gmail_quote">On Aug 23, 2015 7:07 PM, "Rafael Espíndola" <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">SolarisScanLibDirForGCCTriple should start with a lower case. Starting<br>
it with "scan" would probably also be more in line with the code<br>
style.<br>
<br>
LGTM<br>
<br>
On 11 August 2015 at 16:33, Xan López <<a href="mailto:xan@igalia.com">xan@igalia.com</a>> wrote:<br>
> Hi,<br>
><br>
> thanks for the review, I was not even aware that this could be<br>
> tested. Adding a test helped to fix me a couple extra issues (plus the<br>
> one you already mentioned). New patch attached.<br>
><br>
> Xan<br>
><br>
> On Wed, Aug 05, 2015 at 09:14:30AM -0400, Rafael Espíndola wrote:<br>
>> Please git-clang-format this patch.<br>
>><br>
>> +  // /usr/gcc/<major>.<minor>/lib/gcc/<major>.<minor>.<patch>/,<br>
>><br>
>> The code appends a triple after the "/lib/gcc". Is the comment missing it?<br>
>><br>
>> The inner loop has no version comparison. Are you depending on the<br>
>> directory iteration order?<br>
>><br>
>> Can you add a testcase?<br>
>><br>
>><br>
>> On 28 July 2015 at 12:35, Xan López <<a href="mailto:xan@igalia.com">xan@igalia.com</a>> wrote:<br>
>> > Here it is.<br>
>> ><br>
>> > On Tue, Jul 28, 2015 at 01:21:06PM +0200, Xan López wrote:<br>
>> >> On Tue, Jul 28, 2015 at 01:55:23PM +0300, Yaron Keren wrote:<br>
>> >> > +cfe-commits<br>
>> >> ><br>
>> >> > This is a very large Solaris special case in ScanLibDirForGCCTriple which<br>
>> >> > shares almost no code with the function.<br>
>> >> > How about splitting it out to a helper function or<br>
>> >> > making ScanLibDirForGCCTriple virtual and overriding on Solaris?<br>
>> >><br>
>> >> Yep, at least a helper function makes sense, you are right. I'll send<br>
>> >> another patch with either of those suggestions later today.<br>
>> >><br>
>> >><br>
>> >> Xan<br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> > _______________________________________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
</blockquote></div>