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

[David Lindholm]

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/

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