[7u4-osx] Please review: 7124089: launcher refactoring v2.0

[David Holmes]

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. :(

David
-----

> 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:
> http://cr.openjdk.java.net/~ksrini/7124089/webrev.2/webrev.delta/index.html
>
> The full webrev is here:
> http://cr.openjdk.java.net/~ksrini/7124089/webrev.2/index.html
>
> Thanks
> Kumar
>
>> 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)
>> $(SOMEPLACE)
>> 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.
>>
>> -kto
>>
>> 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:
>>>
>>> Highlights:
>>> 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
>>> options
>>>
>>> For those who have already reviewed the 0th version, here is an
>>> incremental webrev to make it easier reviewing the differences.
>>> http://cr.openjdk.java.net/~ksrini/7124089/webrev.1/webrev.delta/index.html
>>>
>>>
>>> Here is the complete webrev:
>>> http://cr.openjdk.java.net/~ksrini/7124089/webrev.1/index.html
>>>
>>> Thanks
>>> Kumar
>>>
>>>
>