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

[Coleen Phillimore]

On 9/9/14, 4:18 PM, Daniel D. Daugherty wrote:
> 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. :-)

There is a reason we don't pretty-print the JVM.  And some style 
variations are more in violation than others.  New code should always 
follow the style guide but existing code shouldn't be reformatted unless 
there is a compelling reason, and nobody gets hurt having to merge.

I did break a lot reformatting by changing klassOop to Klass* but I 
don't think they're worth cleaning up manually unless you are touching 
that code.

Thanks for taking another look.

Coleen

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