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