RFR (S) 8211926: Catastrophic size_t underflow in BitMap::*_large methods
thomas.stuefe at gmail.com
Sat Nov 10 12:58:27 UTC 2018
change looks fine. Some small remarks:
+ assert(beg <= end, "underflow");
I would feel slightly safer with a runtime check for product as well.
The tests favor the left side (lower space) of the bitmap, since l
never grows beyond FUZZ_WINDOW. I wonder whether testing the right
side (small ranges near the bitmap end) would be useful. Maybe with a
gliding window, or just repeat the tests anchored at the right instead
of the left side.
Side notes (nothing to do with your patch):
- BitMap::set_range_within_word, clear_range... : would like to see
asserts here for beg <= end.
- I find it confusing that we use the same type (idx_t) both for word-
and bit-indices. It would make the code a bit more readable if those
were separate types (even if they both were to alias to size_t,
- Generally, I find putting the decision between "large" and
"non-large" APIs off to the caller a bit strange. As a user of bitmap,
I cannot know at which point which variant would be better. I would
prefer the class itself handling that.
All these are idle musings, nothing more. The change is fine to me as it is.
On Sat, Nov 10, 2018 at 12:32 AM Aleksey Shipilev <shade at redhat.com> wrote:
> On 11/09/2018 09:02 PM, Kim Barrett wrote:
> > Almost, but not quite, I'm afraid. All but the first comment below
> > are optional, though I think make things better.
> Introduced helper method, moved the small_range_words to private, handled (l == r) corner case in
> test. See:
> P.S. For the love of any deity of choice, can we please push this corner-case fix and move on to
> solve meatier bugs? I appreciate the attention to details, but the economics of spending any more
> time to this issue feels dubious to me.
More information about the hotspot-dev