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