RFR: 8080775: Better argument formatting for assert() and friends
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