[7u4-osx] Please review: 7124089: launcher refactoring v2.0 (original) (raw)

Kumar Srinivasan kumar.x.srinivasan at oracle.COM
Mon Jan 23 09:33:08 PST 2012

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)/∗)andacp(SOMEDIR)/*) and a cp (SOMEDIR)/∗)andacp(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

More information about the macosx-port-dev mailing list