[7u4-osx] Please review: 7124089: launcher refactoring v2.0
david.holmes at oracle.com
Sun Jan 22 18:14:32 PST 2012
On 22/01/2012 4:05 AM, Kumar Srinivasan wrote:
> Hi Kelly et. al.,
> I have beautified/fixed the Makefiles addressing Kellys' comments below:
> 1. Indented the Makefiles correctly.
By some definition of "correct". The existing indentation is all over
the place with 2, 4 and 8 space indents. The changes seem to use 4 most
often but the end result is still a mix of 2,4 and 8 AFAICS. :(
> 2. Annotated with more trailing comments to the if/else/endif clauses
> 3. Removed the offending \ escapes
> 4. Removed the * from Release.gmk, it turns out the files being copied
> were not quite right (missing files), fixed it such that all the
> appropriate files
> are copied.
> 5. Added comments for the MacOSX specific cflags.
> The incremental webrev is here:
> The full webrev is here:
>> On the Makefiles....
>> Please refrain from using any wildcards (e.g. * ) in the make rules.
>> Better to be explicit, or if necessary
>> use something like FILES=$(wildcard $(SOMEDIR)/*) and a cp $(FILES)
>> so that we can at least see in the Makefile log what it really copied.
>> Please indent the Makefile if/else/endif statements.
>> Thank you for the trailing comments on the endif's. ;^)
>> Please try to avoid escaped quotes on the compile lines, use this
>> -DX='"abc"' rather than this -DX=\"abc\"
>> escaped quotes are very problematic on Windows and I know this isn't
>> Windows, but it tempts windows
>> people to use it, it will not work in all situations. Where '"abc"' does.
>> Please add a comment on what the -Os compiler option means, and also
>> the -x objective-c, I could guess
>> but would be better to document it in the makefile.
>> On Jan 20, 2012, at 8:24 AM, Kumar Srinivasan wrote:
>>> Hi All,
>>> Based on all the comments from Anthony, Joe and David,
>>> here is the modified version:
>>> 1. re-factored code in solaris directory to be shared with macosx,
>>> reducing duplication across the *nixes.
>>> 2. adjusted the makefilesto allow the above
>>> 2. eliminated all conditionals from the shared java.c
>>> 3. added a new launcher regression test for the macosx specific -X
>>> For those who have already reviewed the 0th version, here is an
>>> incremental webrev to make it easier reviewing the differences.
>>> Here is the complete webrev:
More information about the jdk7u-dev