RFR (L/XXS) cleanup indent white space issues (8057107)

[Daniel D. Daugherty]

Thanks for the review!


On 9/9/14 2:05 PM, Coleen Phillimore wrote:
>
> Hi Dan,
>
> I've looked through the changes in the patch file which is mind-numbing.

Which is why all the changes in 8057107 were done with a tool. :-)


> But I don't think you should make any of the changes to 
> sharedRuntime.cpp.  I don't think the change like this is worth the 
> interference with others that have to merge up with this patch 
> (including myself).  For example:
>
>  // Resolves a call.  The compilers generate code for calls that go here
>  // and are patched with the real destination of the call.
>  methodHandle SharedRuntime::resolve_sub_helper(JavaThread *thread,
> -                                           bool is_virtual,
> -                                           bool is_optimized, TRAPS) {
> +                                               bool is_virtual,
> +                                               bool is_optimized, 
> TRAPS) {
>
>
> and
>
> @@ -1213,8 +1213,8 @@
>      bool static_bound = 
> call_info.resolved_method()->can_be_statically_bound();
>      KlassHandle h_klass(THREAD, invoke_code == 
> Bytecodes::_invokehandle ? NULL : receiver->klass());
>      CompiledIC::compute_monomorphic_entry(callee_method, h_klass,
> -                     is_optimized, static_bound, virtual_call_info,
> -                     CHECK_(methodHandle()));
> +                                          is_optimized, static_bound, 
> virtual_call_info,
> + CHECK_(methodHandle()));

The above two are in direct violation of the HotSpot style guide
that John Rose pointed me at. I originally read it a different
way and Mr Simms corrected me that when you have continuation
lines, they are supposed to line up with the closest unmatched
left paren... I had fixed formatting in some other fix and had
to fix it differently... :-)


>
>
> And:
>
>      switch (exception_kind) {
> -      case IMPLICIT_NULL:           return 
> Interpreter::throw_NullPointerException_entry();
> -      case IMPLICIT_DIVIDE_BY_ZERO: return 
> Interpreter::throw_ArithmeticException_entry();
> -      case STACK_OVERFLOW:          return 
> Interpreter::throw_StackOverflowError_entry();
> -      default:                      ShouldNotReachHere();
> +    case IMPLICIT_NULL:           return 
> Interpreter::throw_NullPointerException_entry();
> +    case IMPLICIT_DIVIDE_BY_ZERO: return 
> Interpreter::throw_ArithmeticException_entry();
> +    case STACK_OVERFLOW:          return 
> Interpreter::throw_StackOverflowError_entry();
> +    default:                      ShouldNotReachHere();
>      }
>
> The case's can be indented from switch. I don't see anything wrong 
> with that and it's not worth the disruption to the code.

I'll have to double check the style guide, but I'm fairly certain
that 'case' is supposed to line up with 'switch'.


> And I'm even going to have to merge with this!  I think you should 
> revert sharedRuntime.cpp unless there are egregious formatting 
> violations that I didn't notice in the bulk of changes.

Yes, I understand the merging difficulty... I have to merge with
these changes myself and I have to do it across 9 repos for the
Contended Locking project.

I'll take a separate look at sharedRuntime.cpp and see if I concur
about dropping the changes.


> I thought thread.cpp would have the same merging difficulty for 
> others, but the changes seem to be to areas covered by contended 
> locking improvements (or small) and not likely to cause disruption.

Thanks!

Dan


>
> Thanks,
> Coleen
>
>
>
>
> On 9/7/14, 3:13 PM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have the fix for the following bug ready for JDK9 RT_Baseline:
>>
>>     JDK-8057107 cleanup indent white space issues prior to Contended
>>                 Locking reorder and cache line bucket
>>     https://bugs.openjdk.java.net/browse/JDK-8057107
>>
>> This is both a (L)arge and e(X)tra e(X)tra (S)mall review! Here is the
>> URL for the (L)arge webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8057107-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/8057107-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.
>>
>> These indent white space cleanups were done in preparation for the 
>> reorder
>> and cache line bucket from the Contended Locking project; these changes
>> were made by my do_all_line_filters.ksh script. The script does not fix
>> everything in these files that is not in sync with HotSpot style, but it
>> does fix a large portion of them.
>>
>> 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_all_line_filters.ksh script and the line_indent_filter.java used
>> to do the work to JDK-8057107.
>>
>> Dan
>>
>>
>