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