RFR (M/XXS) cleanup more non-indent white space issues (8047156)

[Daniel D. Daugherty]

On 6/18/14 9:26 AM, Lois Foltan wrote:
>
> On 6/18/2014 9:54 AM, Daniel D. Daugherty wrote:
>> Lois,
>>
>> Thanks for the review. More replies embedded below.
>
> Hi Dan,
>
> Thanks for the explanation.  Any chance you would clean up these two 
> instances manually since you are in those files anyways?

The changes in this bug (JDK-8047156) and the previous bug
(JDK-8046758) are intentionally only the ones made by
do_space_filter.ksh. I have plans for a manual pass, but that
will use a different bug ID that makes it clear that those
changes are manual and not easy to apply in an automated
fashion.


> Either way, I like the clean up and am good with your changes.

Thanks!

Dan


>
> Lois
>
>>
>>
>> On 6/18/14 5:40 AM, Lois Foltan wrote:
>>> Hi Dan,
>>>
>>> Looks good.  One comment,  why wasn't the space removed after the 
>>> function name but before the "(" for the start of the parameters?
>>>
>>> src/share/vm/runtime/mutex.cpp
>>> -#define TRACE(m) { static volatile int ctr = 0 ; int x = ++ctr ; if ((x & (x-1))==0) { ::printf ("%d:%s\n", x, #m); ::fflush(stdout); }}
>>> +#define TRACE(m) { static volatile int ctr = 0; int x = ++ctr; if ((x & (x-1))==0) { ::printf ("%d:%s\n", x, #m); ::fflush(stdout); }}
>>
>> Single line code blocks are difficult to clean. The '<space>;' instances
>> were easy for the script to clean, but the '::printf<space>(' is much
>> harder because it is a function call in the middle of other things that
>> look like function calls ('TRACE(m)' and '::fflush(stdout)')
>>
>>
>>> src/share/vm/runtime/synchronizer.hpp:
>>> -  void waitUninterruptibly (TRAPS) { ObjectSynchronizer::waitUninterruptibly (_obj, 0, CHECK);}
>>> +  void waitUninterruptibly (TRAPS) { ObjectSynchronizer::waitUninterruptibly (_obj, 0, CHECK); }
>>
>> Single line function definitions are difficult, but I would have
>> expected 'waitUninterruptibly<space>(TRAPS)' to work... Hmmm...
>> wait... the 'TRAPS' parameter doesn't match the usual format for
>> parameters in a function definition, i.e., <type> <param>. It's a
>> single word and that's probably why the filter skipped it.
>>
>> The interior 'ObjectSynchronizer::waitUninterruptibly<space>(' is
>> harder for the filter to recognize so it was skipped.
>>
>>
>>>
>>> I'm just curious because there are other places where removing the 
>>> space does occur?  For example:
>>>
>>> src/share/vm/runtime/mutex.cpp
>>> -    const intptr_t u = CASPTR (&_LockWord, v, v|_LBIT) ;
>>> -    if (v == u) return 1 ;
>>> -    v = u ;
>>> +    if ((v & _LBIT) != 0) return 0;
>>> +    const intptr_t u = CASPTR(&_LockWord, v, v|_LBIT);
>>
>> Right. The 'CASPTR(...)' matches the pattern for a function call so
>> it got cleaned up.
>>
>> The big flaw is that my script isn't a real parser. It's a collection
>> of heuristic pattern matchers and filter/edit lines. The real solution
>> is a parser that not only tokenizes C++, but keeps track of the white
>> space associated with each token. Once you have such a tree, you can
>> apply a set of rules that changes/adds whitespace as needed on a per
>> token basis and then outputs the resulting tree as a modified source
>> file.
>>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> Lois
>>>
>>>
>>> On 6/17/2014 9:12 PM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I forgot to update my list of files to clean for the latest version
>>>> of the cleanup bucket so I missed these seven files when I sent out
>>>> the review for:
>>>>
>>>>     JDK-8046758 cleanup non-indent white space issues prior to 
>>>> Contended
>>>>                 Locking cleanup bucket
>>>> https://bugs.openjdk.java.net/browse/JDK-8046758
>>>>
>>>> I have the fix for the following bug ready for JDK9 RT_Baseline:
>>>>
>>>>     JDK-8047156 cleanup more non-indent white space issues prior to
>>>>                 Contended Locking cleanup bucket
>>>> https://bugs.openjdk.java.net/browse/JDK-8047156
>>>>
>>>> This is both a (M)edium and e(X)tra e(X)tra (S)mall review! Here is 
>>>> the
>>>> URL for the (M)edium webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8047156-webrev/0-jdk9-hs-rt/
>>>>
>>>> Since these are 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 e(X)tra e(X)tra (S)mall webrev which was 
>>>> generated with a version of webrev that ignores all whitespace 
>>>> changes:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8047156-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 no non-white space changes. The patch file for
>>>> this webrev shows all the white space changes.
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> At the end of the review cycle for this fix, I will attach the 
>>>> version of the do_space_filter.ksh script used to do the work to 
>>>> JDK-8047156.
>>>>
>>>> Dan
>>>>
>>>>
>>>
>>
>