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

[Per Liden]

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