Request for code review - JDK-8141135,Remove G1RemSet::write_ref

[Thomas Schatzl]

Hi,

On Tue, 2015-11-10 at 15:48 +0100, David Lindholm wrote:
> 
> On 2015-11-10 10:04, Thomas Schatzl wrote:
> > Hi,
> >
> > On Mon, 2015-11-09 at 23:57 -0500, Kim Barrett wrote:
> >> On Nov 9, 2015, at 12:12 PM, Alexander Harlap <alexander.harlap at oracle.com> wrote:
> >>> Remove some unused code:
> >>>
> >>> JDK-8141135
> >>>
> >>> Proposed change:
> >>> http://cr.openjdk.java.net/~aharlap/8141135/webrev.00
> >>>
> >>> Alex
> >> The description of write_ref / par_write_ref doesn't entirely match
> >> the implementation.
> >>
> >> The description says "from" must be non-NULL. However, the assertion
> >> validating "from" and that "p" is within it explicitly checks for and
> >> allows "from" == NULL.
> > That assertion will crash if "from" is NULL. Not sure if crashes
> > indicate being fine to use it.
> >
> > Also one could interpret the ", which is required to be non-NULL" could
> > refer to "p" as well, which is the subject that is talked about. But
> > that is just my interpretation of the sentence... (but I can be
> > convinced that it can be read your way).
> >
> > I think it is only implied that "from" must be non-NULL, since "p" must
> > be non-NULL. If p is NULL, the method will also immediately crash.
> >
> > Maybe move the assert that checks from->is_in_reserved(p) to the top of
> > the method body and add a clause that checks from != NULL?
> 
> This assert does not look like that any more. It now looks like this:
> 
> assert(from->is_in_reserved(p) || from->is_starts_humongous(), "p is not 
> in from");
> 
> There were an assert(_from != NULL) at the only call to this function.

  actually I have already been looking on the current code and assert,
that is different to Alex' baseline.

This is probably where the confusion comes from.

Thanks,
  Thomas