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