RFR(S): 8007772: G1: assert(!hr->isHumongous() || mr.start() == hr->bottom()) failed: the start of HeapRegion and MemRegion should be consistent for humongous regions

[Kirk Pepperdine]

IMHO, (not that it counts for much and that's ok), as I read the code I find that it's very difficult to break into. I've always appreciated comments. I would suggest that if you dup code, you should explain why. If you rely on the short-circut, you'll need to comment why? I found both ways confusing... but then that's just me.

Regards,
Kirk

On 2013-02-11, at 11:56 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:

> OK, so you don't think a comment would avoid miss interpretation?
> I'm fine either way really. I just try to avoid code duplication.
> Ship it!
> /Jesper
> 
> 
> 11 feb 2013 kl. 23:35 skrev John Cuthbertson <john.cuthbertson at oracle.com>:
> 
>> Hi Jesper,
>> 
>> On 2/11/2013 12:51 PM, Jesper Wilhelmsson wrote:
>>> I guess it's a matter of taste, to me it is a lot clearer. But sure, it takes a second look at the code to see that short-circuiting matters here.
>>> 
>>> The version with code duplication is a bit confusing to me and requires a second look to see that there is no difference between the different clauses in the if/else if.
>>> 
>>> I would argue that for someone who is new to the code, the non-duplicated code would be easier to get familiar with due to (at a first glance) less complicated code. And you don't have to know that the short-circuiting is necessary until you know the code well enough to realize that it is necessary, if you see what I mean.
>>> 
>>> We could also add a comment :-)
>>> /Jesper
>>> 
>> 
>> We could argue but I don't think that you will convince me that your suggestion is clearer. I first read the code the same way as Bengt and was going to make a similar comment as his until an "Aha!" moment and I saw the short-circuit. Reading the code incorrectly has biased me. :)
>> 
>> Thanks,
>> 
>> JohnC