code review round 1 for Full Debug Symbols on MacOS X hotspot (7165611)

[Daniel D. Daugherty]

On 10/15/13 8:50 AM, Daniel D. Daugherty wrote:
> On 10/15/13 6:21 AM, Magnus Ihse Bursie wrote:
>> On 2013-10-11 22:27, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I'm ready for code review round 1 of the FDS on MacOS X hotspot 
>>> changes.
>>> Below is the original code review round 0 invite (slightly edited for
>>> clarity). Working on FDS is like pulling a thread on a sweater... so
>>> there are four additional changed files along with more changes to
>>> many of the files from round 0.
>>>
>>> Here is the code review round 1 JDK8/HSX-25 webrev URL:
>>>
>>> OpenJDK: 
>>> http://cr.openjdk.java.net/~dcubed/fds_revamp/7165611-webrev/1-jdk8/
>>> Internal: 
>>> http://javaweb.us.oracle.com/~ddaugher/fds_revamp/7165611-webrev/1-jdk8/ 
>>>
>>
>> New build system parts overall looks good. Just one note: Your webrev 
>> is based on the code before the whitespace/indentation cleanup. 
>> Please try to keep to the new indentation policy (see 
>> http://mail.openjdk.java.net/pipermail/build-dev/2013-October/010477.html) 
>> when merging. That means e.g. that code like:
>>
>>         ifneq ($(OPENJDK_TARGET_OS), macosx)   # OBJCOPY is not used 
>> on MacOS X
>>         ifneq ($(OPENJDK_TARGET_OS), windows)  # nor on Windows
>
> Magnus,
>
> Thanks for the heads up on the style update. I will fix the above
> two lines to meet the new guidelines.
>
>
>> shoud be properly indented and not on the same level.
>
> I'm guessing that the logic from which I copied this style will be
> fixed in the upcoming Makefile push from the build-infra team. I'm
> in sync with JDK8-B111/HSX-25-B54 so I'm guessing that these changes
> haven't been pushed yet.
>
> Dan
>
>
>>
>> /Magnus

Here's the diff:

$ diff common/makefiles/NativeCompilation.gmk.cr1 
common/makefiles/NativeCompilation.gmk
439c439
<                 ifneq ($(OPENJDK_TARGET_OS), windows)  # nor on Windows
---
 >                   ifneq ($(OPENJDK_TARGET_OS), windows)  # nor on Windows
463c463
<                 endif # !windows
---
 >                   endif # !windows
523c523
<                 ifneq ($(OPENJDK_TARGET_OS), windows)  # nor on Windows
---
 >                   ifneq ($(OPENJDK_TARGET_OS), windows)  # nor on Windows
547c547
<                 endif # !windows
---
 >                   endif # !windows


I took a quick scan of the rest of patch and I didn't see anything
jump out at me style-wise. If I missing something, we can follow up
with another bug.

Dan

PS
As a HotSpot guy, I'm happy to see the indent migrating from
4 spaces (or more) down to 2 space...