RFR (L/S) manual cleanup of white space issues (8057109)

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Tue Sep 9 00:17:55 UTC 2014


Nice cleanup!
It looks good.
Some minor comments below.


On 9/8/14 3:01 PM, Daniel D. Daugherty wrote:
> Updated after Fred's code review:
>
> Here is the URL for the (L)arge webrev:
>
> http://cr.openjdk.java.net/~dcubed/8057109-webrev/1-jdk9-hs-rt/
>
> Here is the URL for the (S)mall webrev which was generated with a version
> of webrev that ignores all whitespace changes:
>
> http://cr.openjdk.java.net/~dcubed/8057109-webrev/1-jdk9-hs-rt-no_ws/
>
> Dan
>
>
> On 9/7/14 1:13 PM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have the fix for the following bug ready for JDK9 RT_Baseline:
>>
>>     JDK-8057109 manual cleanup of white space issues prior to Contended
>>                 Locking reorder and cache line bucket
>>     https://bugs.openjdk.java.net/browse/JDK-8057109
>>
>> This is both a (L)arge and a (S)mall review! Here is the URL for the
>> (L)arge webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8057109-webrev/0-jdk9-hs-rt/

src/os/solaris/vm/os_solaris.cpp

   4022   assert((chainedsigactions != (struct sigaction *)NULL) && (preinstalled_sigs != (int *)NULL), "signals not yet initialized");

    The line above is worth to split as it is done for 4032-4033.


src/os/windows/vm/os_windows.cpp:

  196       *(strrchr(home_dir, '\\')) = '\0';  // get rid of \jvm.dll
  197       pslash = strrchr(home_dir, '\\');
  198       if (pslash != NULL) {
  199         *pslash = '\0';                 // get rid of \{client|server}
  200         pslash = strrchr(home_dir, '\\');
  201         if (pslash != NULL) {
  202           *pslash = '\0';             // get rid of \bin
  203         }

   Minor: It is better to align the comments.


Thanks,
Serguei

>>
>> Since these are mostly white space fixes, I recommend the udiff links
>> instead of the frames links. Of course, some folks prefer the patch file
>> itself for white space fixes.
>>
>> Here is the URL for the (S)mall webrev which was generated with a 
>> version
>> of webrev that ignores all whitespace changes:
>>
>> http://cr.openjdk.java.net/~dcubed/8057109-webrev/0-jdk9-hs-rt-no_ws/
>>
>> Again, I recommend the udiff links since that's the fastest way to see
>> that webrev_no_ws found minimal non-white space changes. The patch file
>> for this webrev shows all the changes.
>>
>> These manual changes were motivated by various review comments made for
>> earlier fixes in the Contended Locking project. During those reviews, I
>> deferred making some of the requested format changes to this round of
>> manual changes. In most cases, the reviewers posted comments with one
>> or two examples of things they did not like and I've swept all 17 files
>> that we've targeted for cleanup in the Contended Locking project looking
>> for those style problems and fixing them. I've made a total of 12 passes
>> over the 17 files, but I cannot say that I've caught everything. Most
>> stuff that annoys people is fixed, but it's very difficult to catch and
>> fix them all without a real parser based solution.
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>>
>>
>>
>



More information about the hotspot-runtime-dev mailing list