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

[Daniel D. Daugherty]

Lois,

Thanks for the review. More replies embedded below.


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