Review request for OPENJDK6-40: OpenJDK6-b32 does not compile the testcase that Oracle JDK 6u45 compiles fine

[Andrew Hughes]

----- Original Message -----
> Hi Andrew,
> 
> On 15.12.2014 18:40, Andrew Hughes wrote:
> > The main thing that concerns me is the loss of the original author
> > information, but I'm not sure if we have a solution for this as yet.
> > I tend to go with the original author as the changeset author
> > (hg commit --user <username>) but that also has the downside of
> > not crediting the backporter.
> >
> > I guess both it and the original reviewer could be looked up via
> > the changeset in 7, which is:
> >
> > 6938454: Unable to determine generic type in program that compiles under
> > Java 6
> > Summary: a redundant dubtyping check causes spurious inference failure
> > Reviewed-by: jjg
> >
> > I think that's meant to be 'subtyping' ;)
> 
> As we discussed some time ago, for backports we could add a new
> commit message header like "Authored-by: <original fix author>",
> but it likely requires jcheck modification and updating official
> rules.

Yes, I remember this discussion. I was hoping someone might be able to
provide feedback on any progress on getting this supported in jcheck, as
I've not heard of it being added.

> 
> > I hadn't realised this. Is there a reason the following hunks are absent?
> >
> > diff -r ed354a00f76b -r 36c4ec4525b4
> > src/share/classes/com/sun/tools/javac/comp/Attr.java
> > --- a/src/share/classes/com/sun/tools/javac/comp/Attr.java	Tue Jul 27
> > 11:52:11 2010 -0700
> > +++ b/src/share/classes/com/sun/tools/javac/comp/Attr.java	Thu Jul 29
> > 15:56:25 2010 +0100
> > @@ -1694,8 +1694,22 @@
> >               //if the type of the instance creation expression is an
> >               interface
> >               //skip the method resolution step (JLS 15.12.2.7). The type
> >               to be
> >               //inferred is of the kind <X1,X2, ... Xn>C<X1,X2, ... Xn>
> > -            clazztype = new ForAll(clazztype.tsym.type.allparams(),
> > -                    clazztype.tsym.type);
> > +            clazztype = new ForAll(clazztype.tsym.type.allparams(),
> > clazztype.tsym.type) {
> > +                @Override
> > +                public List<Type> getConstraints(TypeVar tv,
> > ConstraintKind ck) {
> > +                    switch (ck) {
> > +                        case EXTENDS: return types.getBounds(tv);
> > +                        default: return List.nil();
> > +                    }
> > +                }
> > +                @Override
> > +                public Type inst(List<Type> inferred, Types types) throws
> > Infer.NoInstanceException {
> > +                    // check that inferred bounds conform to their bounds
> > +                    infer.checkWithinBounds(tvars,
> > +                           types.subst(tvars, tvars, inferred),
> > Warner.noWarnings);
> > +                    return super.inst(inferred, types);
> > +                }
> > +            };
> >           } else {
> >               //if the type of the instance creation expression is a class
> >               type
> >               //apply method resolution inference (JLS 15.12.2.7). The
> >               return type
> >
> > diff -r ed354a00f76b -r 36c4ec4525b4
> > src/share/classes/com/sun/tools/javac/comp/Infer.java
> > --- a/src/share/classes/com/sun/tools/javac/comp/Infer.java	Tue Jul 27
> > 11:52:11 2010 -0700
> > +++ b/src/share/classes/com/sun/tools/javac/comp/Infer.java	Thu Jul 29
> > 15:56:25 2010 +0100
> > @@ -458,7 +457,7 @@
> >
> >       /** check that type parameters are within their bounds.
> >        */
> > -    private void checkWithinBounds(List<Type> tvars,
> > +    void checkWithinBounds(List<Type> tvars,
> >                                      List<Type> arguments,
> >                                      Warner warn)
> >           throws InvalidInstanceException {
> 
> These parts of the original fix were excluded because they are
> related to diamond ("<>") operator support, which appeared in
> Java 7 and is not a part of Java 6.
> 

Ah, ok.

> > Do the new tests added by the change pass?
> 
> Yes, I checked newly added testcases, and also all existing
> javac jtreg tests (langtools/test/tools/javac) - all of them passed.
> 

Ok, this looks good to go.

> Thanks,
> Nikolay
> 
> >
> >> Thanks,
> >>
> >> Ivan
> >>
> >> 6938454: Unable to determine generic type in program that compiles under
> >> Java 6
> >> Reviewed-by: aph
> >> Contributed-by: nikgor <nikolay at azulsystems.com>
> >>
> >>
> >>
> >> On 11/12/2014 22:01, Andrew Haley wrote:
> >>> On 12/11/2014 06:59 PM, Nikolay Gorshkov wrote:
> >>>> Me and my colleagues have a consensus that the patch is important to us
> >>>> -
> >>>> we are responsible for introducing a regression into OpenJDK 6 (breaking
> >>>> Apache Flume 1.5.0 compilation), and so we have to fix it. We have
> >>>> already
> >>>> included this patch into our OpenJDK 6 -based products, and now we are
> >>>> interested in upstreaming it to OpenJDK 6 itself to minimize the
> >>>> difference
> >>>> in codebase.
> >>> Reluctantly, OK.  I am crossing my fingers and hoping that this
> >>> is the last we'll see of this bug!  We shall see.
> >>>
> >>> Thanks for your patience,
> >>> Andrew.
> >>>
> >>>
> >>
> >>
> >
> > Thanks,
> >
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07