RFR: 8080775: Better argument formatting for assert() and friends
David Lindholm
david.lindholm at oracle.com
Thu Sep 24 08:21:10 UTC 2015
Kim,
Thanks again for looking at this. I have fixed your comments below, and
even though you have some files left I have uploaded a new webrev with
the changes I have made so far:
http://cr.openjdk.java.net/~david/JDK-8080775/webrev.01/
http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00-01/ (differential)
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 * 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