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

Kim Barrett kim.barrett at oracle.com
Wed Sep 23 18:32:47 UTC 2015

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 * word_sz == fc->size(), 2878 "Chunk size " SIZE_FORMAT " is not exactly splittable by " 2879 SIZE_FORMAT " sized chunks of size " SIZE_FORMAT, 2880 fc->size(), n, word_sz);

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), num_narrow, num_full, do_oop_order)

194 assert((got) == (expected),
195 "Expected: %d, got: %d. testIsBufferEmptyOrFull(%d, %d, %s, %s)",
196 (got), (expected), num_narrow, num_full,
197 BOOL_TO_STR(expect_empty), BOOL_TO_STR(expect_full))

234 assert(boc.is_buffer_empty(), 235 "Should be empty after call to done(). testEmptyAfterDone(%d, %d)", 236 num_narrow, num_full);

Pre-existing: Indentation of message arguments.

src/share/vm/gc/g1/g1AllocRegion.hpp 37 #define G1_ALLOC_REGION_MSG(message)
38 "[%s] %s c: %u b: %s r: " PTR_FORMAT " u: " SIZE_FORMAT,
39 _name, message, _count, BOOL_TO_STR(_bot_updates),
40 p2i(_alloc_region), _used_bytes_before

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 G1_ALLOC_REGION_ASSERT, 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 VALUE_OBJ_CLASS_SPEC { 50 friend class ar_ext_msg;

ar_ext_msg no longer exists.

src/share/vm/gc/g1/g1Allocator.cpp 432 assert((end_alignment_in_bytes >> LogHeapWordSize) < HeapRegion::min_region_size_in_words(), 433 "alignment " SIZE_FORMAT " too large", end_alignment_in_bytes);

Pre-existing: Indentation of message arguments.

src/share/vm/gc/g1/g1BiasedArray.cpp 44 guarantee(biased_index >= bias() && biased_index < (bias() + length()), 45 "Biased index out of bounds, index: " SIZE_FORMAT " bias: " SIZE_FORMAT " length: " SIZE_FORMAT, biased_index, bias(), length());

50 guarantee(biased_index >= bias() && biased_index <= (bias() + length()), 51 "Biased index out of inclusive bounds, index: " SIZE_FORMAT " bias: " SIZE_FORMAT " length: " SIZE_FORMAT, biased_index, bias(), length());

Pre-existing: Indentation of message arguments.

Maybe also split really long lines?

src/share/vm/gc/g1/g1BlockOffsetTable.cpp 364 assert((_array->offset_array(orig_index) == 0 && 365 blk_start == boundary) || 366 (_array->offset_array(orig_index) > 0 && 367 _array->offset_array(orig_index) <= N_words),

[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 <= N_words, 152 "%s - " 153 "offset: " SIZE_FORMAT ", N_words: %u", 154 msg, offset, (uint)N_words);

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 " PTR_FORMAT 97 " _reserved.start() " PTR_FORMAT " _reserved.end() " 98 PTR_FORMAT, 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(_old_marking_cycles_started == _old_marking_cycles_completed || 2541 _old_marking_cycles_started == _old_marking_cycles_completed + 1, 2542 "Wrong marking cycle count (started: %d, completed: %d)", 2543 _old_marking_cycles_started, _old_marking_cycles_completed);

Pre-existing: Indentation of arguments.

src/share/vm/gc/g1/heapRegionSet.cpp

Suggest eliminating hrs_ext_msg and hrs_err_msg in a manner similar to elimination of ar_ext_msg, 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 hrt_assert_is_valid(tag)
31 assert(is_valid((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 NOT_DEBUG_RETURN 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(words_to_fill >= CollectedHeap::min_fill_size(), 88 "Remaining size (" SIZE_FORMAT ") is too small to fill (based on " SIZE_FORMAT " and " SIZE_FORMAT ")", 89 words_to_fill, words_left_to_fill, CollectedHeap::filler_array_max_size());

Pre-existing: Indentation of message arguments.

src/share/vm/gc/shared/gcCause.hpp 97 assert(cause != GCCause::_old_generation_too_full_to_scavenge && 98 cause != GCCause::_old_generation_expanded_on_last_scavenge, 99 "This GCCause may be correct but is not expected yet: %s", 100 to_string(cause));

Pre-existing: Indentation of arguments.

src/share/vm/gc/shared/space.cpp 606 assert(oop(last)->is_oop(),PTR_FORMAT " should be an object start", p2i(last));

Add space between comma and PTR_FORMAT.

More information about the hotspot-dev mailing list