RFR: 8080775: Better argument formatting for assert() and friends

[David Lindholm]

Per,

Thank you for the review!

/David

On 2015-09-29 09:55, Per Liden wrote:
> Hi David,
>
> On 2015-09-28 17:37, David Lindholm wrote:
>> Kim and Per,
>>
>> Thank you! I have made fixes for all your comments, the latest webrev is
>> available here:
>>
>> http://cr.openjdk.java.net/~david/JDK-8080775/webrev.03/
>
> Looks good.
>
> /Per
>
>>
>> Kim, I have left the casts in the code, as you say this should be fixed
>> in another change.
>>
>>
>> Thanks,
>> David
>>
>> On 2015-09-28 16:48, Kim Barrett wrote:
>>> This is the last of my comments on:
>>> http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00/
>>>
>>> A few more minor formatting nits, and a couple of things that should
>>> probably be left for later.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/oops/klassVtable.cpp
>>> 1464     fatal("klass %s: klass object too short (vtable extends 
>>> beyond "
>>> 1465           "end)", _klass->internal_name());
>>>
>>> Remove line break in format string.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/oops/method.cpp
>>>   249   { ResourceMark rm;
>>>   250   assert(is_native() && bcp == code_base() || contains(bcp) ||
>>> is_error_reported(),
>>>   251          "bcp doesn't belong to this method: bcp: "
>>> INTPTR_FORMAT ", method: %s", bcp, name_and_sig_as_C_string());
>>>   252   }
>>>
>>> [Pre-existing]
>>> The assert isn't properly indented within the nested block. Note that
>>> I think the ResourceMark is required, due to
>>> name_and_sig_as_C_String().  [Recall that failed assert can still
>>> return, though I think only in non-PRODUCT mode.]
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/oops/method.cpp
>>>   282   assert((is_native() && bci == 0)  || (!is_native() && 0 <= bci
>>> && bci < code_size()), "illegal bci: %d", bci);
>>>
>>> [Pre-existing]
>>> Extra space before "||".
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/oops/method.cpp
>>>   575   if (class_access_flags.is_interface()) {
>>>   576       assert(is_nonv == is_static(), "is_nonv=%s",
>>> name_and_sig_as_C_string());
>>>   577   }
>>>
>>> [Pre-existing]
>>> Indentation of assert statement should be 2, not 4.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/oops/oop.cpp
>>>   126   guarantee(obj->is_oop_or_null(), "invalid oop: "
>>> INTPTR_FORMAT, p2i((oopDesc*) obj));
>>>
>>> [Pre-existing]
>>> I don't think the cast is needed here.
>>>
>>> [Maybe this sort of thing should be left to separate cleanup of p2i
>>> usage.  See below.]
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/oops/oop.inline.hpp
>>>   192   assert(check_obj_alignment(result), "address not aligned: "
>>> INTPTR_FORMAT, p2i((void*) result));
>>>
>>> [Pre-existing]
>>> I don't think the cast is needed here.
>>>
>>> [Maybe this sort of thing should be left to a separate cleanup of p2i
>>> usage.  With two in a row now, I'm inclined to suggest that.]
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/opto/castnode.cpp
>>>   132               fatal("unexpected comparison %s", ss.as_string());
>>>
>>> [Pre-existing. Any change related to this should be separate from the
>>> assert formatting arguments change.]
>>>
>>> [Note that this change removed err_msg_res.]
>>>
>>> There's no obvious associated ResourceMark.  But maybe there should
>>> be, since ss.as_string() performs resource allocation.
>>>
>>> Recall that fatal() and other related macros can return under some
>>> circumstances. (Though I think only non-PRODUCT.)
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/opto/graphKit.hpp
>>>   356     assert(value->bottom_type()->basic_type() == T_INT,
>>>   357         "wrong type: %s",
>>> type2name(value->bottom_type()->basic_type()));
>>> ...
>>>   361     assert(value->bottom_type()->basic_type() == T_LONG,
>>>   362         "wrong type: %s",
>>> type2name(value->bottom_type()->basic_type()));
>>>
>>> Indentation of message.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>>
>>