Erik Joelsson (erik.joels...@oracle.com) wrote: > New webrev: > http://cr.openjdk.java.net/~erikj/7141244/webrev.02/ > <http://cr.openjdk.java.net/%7Eerikj/7141244/webrev.02/> > 177 lines changed: 89 ins; 29 del; 59 mod; 3970 unchg > > Changes since last time: > > * Moved the , to after $(SPEC) > * Changed comment in gcc/sparcWorks.make according to suggestion from > Fredrik.
Looks good. One minor request: in linux/makefiles/gcc.make, you moved the setting of STRIP under the SPEC conditional. Might as well fold it into the CROSS_COMPILE_ARCH conditional that's already there. -John > On 2012-02-09 10:09, Erik Joelsson wrote: > > > > On 2012-02-09 03:51, David Holmes wrote: > >> make/defs.make: > >> > >> + ifneq (,$(SPEC)) > >> + include $(SPEC) > >> + endif > >> > >> Having the blank first looks odd. I assume you aren't using -inlcude > >> because you want to see errors if SPEC is set but not found. > >> > > I guess it's an unconscious habit from java where you rather do > > "".equals(something) to avoid NPE. I will switch it around. And the > > assumption is correct. We used -include at first, but I figured that > > we wanted to know if the include failed at least on the root level > > Makefile. > >> make/windows/makefiles/compile.make: > >> > >> The definitions of MT=mt.exe in each block for the different VS > >> versions seems redundant. If we factor this out is there any reason > >> not to group: > >> > >> CXX=cl.exe > >> MT=mt.exe > >> RC=rc.exe > >> LD=link.exe > >> > >> together and use the same "if (,$(SPEC))" approach? > >> > > Grouping them together would certainly look nicer, but MT isn't set > > for every possible compiler version. Not sure if that matters since we > > don't support older versions anyway, right? > > > > As for testing for SPEC, this is nmake and the SPEC file is only > > gnumake compatible. CXX, MT, RC and LD are sent in to nmake on the > > command line from gnumake. They are then generated into local.make > > which is in turn included by sub invocations of nmake. Sending in SPEC > > as well seemed redundant to me, but we could send it in as a flag > > signaling that configure should be in control. Wouldn't look obviously > > better to me though. I'm open for suggestions. > > > > /Erik