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

David Lindholm david.lindholm at oracle.com
Mon Sep 28 15:37:15 UTC 2015

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->internalname()); Remove line break in format string. ------------------------------------------------------------------------------ src/share/vm/oops/method.cpp 249 { ResourceMark rm; 250 assert(isnative() && bcp == codebase() || contains(bcp) || iserrorreported(), 251 "bcp doesn't belong to this method: bcp: " INTPTRFORMAT ", method: %s", bcp, nameandsigasCstring()); 252 } [Pre-existing] The assert isn't properly indented within the nested block. Note that I think the ResourceMark is required, due to nameandsigasCString(). [Recall that failed assert can still return, though I think only in non-PRODUCT mode.] ------------------------------------------------------------------------------ src/share/vm/oops/method.cpp 282 assert((isnative() && bci == 0) || (!isnative() && 0 <= bci && bci < codesize()), "illegal bci: %d", bci);_ _[Pre-existing]_ _Extra space before "||"._ _------------------------------------------------------------------------------_ _src/share/vm/oops/method.cpp_ _575 if (classaccessflags.isinterface()) {_ _576 assert(isnonv == isstatic(), "isnonv=%s", nameandsigasCstring());_ _577 }_ _[Pre-existing]_ _Indentation of assert statement should be 2, not 4._ _------------------------------------------------------------------------------_ _src/share/vm/oops/oop.cpp_ _126 guarantee(obj->isoopornull(), "invalid oop: " INTPTRFORMAT, 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(checkobjalignment(result), "address not aligned: " INTPTRFORMAT, 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.asstring()); [Pre-existing. Any change related to this should be separate from the assert formatting arguments change.] [Note that this change removed errmsgres.] There's no obvious associated ResourceMark. But maybe there should be, since ss.asstring() 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->bottomtype()->basictype() == TINT, 357 "wrong type: %s", type2name(value->bottomtype()->basictype())); ... 361 assert(value->bottomtype()->basictype() == TLONG, 362 "wrong type: %s", type2name(value->bottomtype()->basictype())); Indentation of message. ------------------------------------------------------------------------------

More information about the hotspot-dev mailing list