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

[Kelly O'Hair]

I think you went above and beyond what I was thinking of, sorry if I mis-communicated.
I didn't mean to ask for all else/endif's to get a trailing comment, I just add them when it helps
clarify things when the nesting or length gets bad.
I feel like I created more work for you than necessary, sorry.

And you didn't need to re-indent anything you didn't actually change.

I do think Release.gmk looks better.

But it looks fine.  I would have preferred indentation by 2, but not a big deal.
Some kind of indentation is better than none.

-kto

On Jan 21, 2012, at 10:05 AM, Kumar Srinivasan wrote:

> Hi Kelly et. al.,
> 
> I have beautified/fixed the Makefiles addressing Kellys' comments below:
> 
> 1. Indented the Makefiles correctly.
> 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
>>> 
>>> 
>