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

Per Liden per.liden at oracle.com
Fri Sep 25 06:15:10 UTC 2015

Hi,

On 2015-09-24 16:25, David Lindholm wrote:

Hi Per,

Thanks for the review! Comments inline.

On 2015-09-24 14:56, Per Liden wrote: This is a really nice cleanup! Just a few comments: Thanks!

g1AllocRegion.inline.hpp: ------------------------- Could we rename G1ALLOCREGIONASSERT() to something like assertallocregion(), to match the style used by assertheaplocked() and friends in g1CollectedHeap.hpp. Fixed. heapRegionSet.hpp/heapRegionSet.cpp: ------------------------------------ It feels like we should get rid of hrserrmsg and hrsextmsg here also, and replace it with an assertheapregionset() or something, or is there a reason why we're keeping those? Yes, per Kims suggestion I will add another RFE to fix this in another changeset. vmError.cpp: ------------ VMError::currentstep could be a local variable in report() instead of a static. Yes, but the local variable must still be static, otherwise crashreporting during crashreporting won't work.

Ah, ok. In that case I think it's better to leave next to current_step_info, since that are tightly connected to each other.

cheers, /Per

VMError::currentstepinfo could be reset to "" in the END macro, instead of outside of report(). Yes, I reset both currentstepinfo and currentstep in END now. VMError::verbose could be an argument to report() instead of a static. Fixed. Thanks, David

cheers, /Per

I will also file a new RFE to fix src/share/vm/gc/g1/heapRegionSet.cpp per your suggestion. Thanks, David

On 2015-09-23 20:32, Kim Barrett wrote: On Sep 22, 2015, at 7:38 AM, David Lindholm <david.lindholm at oracle.com> wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8080775 Webrev: http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00/ Another batch, this time covering gc through memory. I need to take a break from this, but I should be able to finish tomorrow. Mostly formatting issues. BTW, although I'm finding some of these, I'm also noticing that you tidied up a lot more. Thanks for that. ------------------------------------------------------------------------------

src/share/vm/gc/cms/compactibleFreeListSpace.cpp 2877 assert(n * wordsz == fc->size(), 2878 "Chunk size " SIZEFORMAT " is not exactly splittable by " 2879 SIZEFORMAT " sized chunks of size " SIZEFORMAT, 2880 fc->size(), n, wordsz); Pre-existing: Indentation of message arguments. ------------------------------------------------------------------------------ src/share/vm/gc/g1/bufferingOopClosure.cpp 157 assert((got) == _(expected), _ 158 "Expected: %d, got: %d, when running testCount(%d, _%d, %d)", _ 159 (got), (expected), numnarrow, numfull, doooporder) 194 assert((got) == _(expected), _ 195 "Expected: %d, got: %d. testIsBufferEmptyOrFull(%d, _%d, %s, %s)", _ 196 (got), (expected), numnarrow, _numfull, _ 197 BOOLTOSTR(expectempty), BOOLTOSTR(expectfull)) 234 assert(boc.isbufferempty(), 235 "Should be empty after call to done(). testEmptyAfterDone(%d, %d)", 236 numnarrow, numfull); Pre-existing: Indentation of message arguments. ------------------------------------------------------------------------------ src/share/vm/gc/g1/g1AllocRegion.hpp _37 #define G1ALLOCREGIONMSG(message) _ _38 "[%s] %s c: %u b: %s r: " PTRFORMAT " u: " SIZEFORMAT, _ _39 name, message, count, BOOLTOSTR(botupdates), _ 40 p2i(allocregion), usedbytesbefore Misaligned line continuations. I think this doesn't belong in this header; it's not something that a client can use. Suggest moving it to g1AllocRegion.inline.hpp. Suggest changing this to G1ALLOCREGIONASSERT, with predicate argument and full assert expression as expansion. Macros with unusual syntactic expansions can be confusing, so best avoided unless they provide some benefit that can't be obtained in some other way. ------------------------------------------------------------------------------ src/share/vm/gc/g1/g1AllocRegion.hpp 49 class G1AllocRegion VALUEOBJCLASSSPEC { 50 friend class arextmsg; arextmsg no longer exists. ------------------------------------------------------------------------------ src/share/vm/gc/g1/g1Allocator.cpp 432 assert((endalignmentinbytes >> LogHeapWordSize) <_ _HeapRegion::minregionsizeinwords(),_ _433 "alignment " SIZEFORMAT " too large",_ _endalignmentinbytes);_ _Pre-existing: Indentation of message arguments._ _------------------------------------------------------------------------------_ _src/share/vm/gc/g1/g1BiasedArray.cpp_ _44 guarantee(biasedindex >= bias() && biasedindex < (bias() +_ _length()),_ _45 "Biased index out of bounds, index: " SIZEFORMAT " bias: "_ _SIZEFORMAT " length: " SIZEFORMAT, biasedindex, bias(), length());_ _50 guarantee(biasedindex >= bias() && biasedindex <= (bias() +_ _length()),_ _51 "Biased index out of inclusive bounds, index: " SIZEFORMAT_ _" bias: " SIZEFORMAT " length: " SIZEFORMAT, biasedindex, bias(),_ _length());_ _Pre-existing: Indentation of message arguments._ _Maybe also split really long lines?_ _------------------------------------------------------------------------------_ _src/share/vm/gc/g1/g1BlockOffsetTable.cpp_ _364 assert((array->offsetarray(origindex) == 0 && 365 blkstart == boundary) || 366 (array->offsetarray(origindex) > 0 && 367 array->offsetarray(origindex) <= Nwords),_ _[Pre-existing, and not modified by this changeset, but nearby.]_ _Indentation of 366 and 367 are wrong._ _------------------------------------------------------------------------------_ _src/share/vm/gc/g1/g1BlockOffsetTable.hpp_ _151 assert(offset <= Nwords,_ _152 "%s - "_ _153 "offset: " SIZEFORMAT ", Nwords: %u",_ _154 msg, offset, (uint)Nwords);_ _Maybe eliminate the line break in the format string?_ _------------------------------------------------------------------------------_ _src/share/vm/gc/g1/g1BlockOffsetTable.inline.hpp_ _95 assert(result >= reserved.start() && result < reserved.end(),_ _96 "bad address from index result " PTRFORMAT_ _97 " reserved.start() " PTRFORMAT " reserved.end() "_ _98 PTRFORMAT,_ _99 p2i(result), p2i(reserved.start()),_ _p2i(reserved.end()));_ _Maybe eliminate the second line break in the format string?_ _------------------------------------------------------------------------------_ _src/share/vm/gc/g1/g1CollectedHeap.cpp_ _2540 assert(oldmarkingcyclesstarted ==_ _oldmarkingcyclescompleted ||_ _2541 oldmarkingcyclesstarted == oldmarkingcyclescompleted_ _+ 1,_ _2542 "Wrong marking cycle count (started: %d, completed: %d)",_ _2543 oldmarkingcyclesstarted, oldmarkingcyclescompleted);_ _Pre-existing: Indentation of arguments._ _------------------------------------------------------------------------------_ _src/share/vm/gc/g1/heapRegionSet.cpp_ _Suggest eliminating hrsextmsg and hrserrmsg in a manner similar to_ _elimination of arextmsg, taking into account earlier comments in_ _that area._ _Please do that under a new RFE; this change set is quite larg enough_ _already._ _------------------------------------------------------------------------------_ _src/share/vm/gc/g1/heapRegionType.hpp_ _30 #define hrtassertisvalid(tag) _ _31 assert(isvalid((tag)), "invalid HR type: %u", (uint) (tag))_ _Pre-existing: I don't think this should be a macro. I suggest it_ _should be a private member function using NOTDEBUGRETURN and an_ _ASSERT-only definition in the .cpp._ _https://bugs.openjdk.java.net/browse/JDK-8137046_ _------------------------------------------------------------------------------_ _src/share/vm/gc/parallel/mutableNUMASpace.cpp_ _87 assert(wordstofill >= CollectedHeap::minfillsize(), 88 "Remaining size (" SIZEFORMAT ") is too small to fill (based on " SIZEFORMAT " and " SIZEFORMAT ")", 89 wordstofill, wordslefttofill, CollectedHeap::fillerarraymaxsize()); Pre-existing: Indentation of message arguments. ------------------------------------------------------------------------------ src/share/vm/gc/shared/gcCause.hpp 97 assert(cause != GCCause::oldgenerationtoofulltoscavenge && 98 cause != GCCause::oldgenerationexpandedonlastscavenge, 99 "This GCCause may be correct but is not expected yet: %s", 100 tostring(cause)); Pre-existing: Indentation of arguments. ------------------------------------------------------------------------------ src/share/vm/gc/shared/space.cpp 606 assert(oop(last)->isoop(),PTRFORMAT " should be an object start", p2i(last)); Add space between comma and PTRFORMAT. ------------------------------------------------------------------------------

More information about the hotspot-dev mailing list