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

[Kumar Srinivasan]

On 1/23/2012 9:16 AM, Kelly O'Hair wrote:
> 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.

No the indentation ticked me off as well, as I could not parse the 
conditionals.
>
> 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.

Right I was in the middle of firing away a response to the occupants of
221b Baker Street, and you answered some of my questions in your
previous email.

I will reset the indentation to 2, to be consistent,  I have trouble 
with tabs,
these days my editor of choice has been NB and it does not help any to
edit makefiles, as I have set it to convert tabs to spaces for real code.

Therefore,  in order to insert tabs for makefiles, I switch back and forth
between gvim.

I think it is time someone came up under the build project to document
coding conventions for makefiles, and *ahem* a jcheck for Makefiles
perhaps ?

One more webrev coming up, later today.

Kumar

>
> -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
>>>>
>>>>