Looks good to me as well! I'll sponsor this and push it.
Best regards, Volker On Wed, Feb 5, 2020 at 6:47 AM Roger Riggs <roger.ri...@oracle.com> wrote: > > Looks good, Thanks > > On 2/4/20 8:06 PM, Verghese, Clive wrote: > > Thank you for the quick feedback. > > > > I have updated the Webrev > > http://cr.openjdk.java.net/~alvdavi/webrevs/8235699/webrev.8u.jdk.06/ > > > > Regards, > > Clive Verghese > > > > On 2/4/20, 4:42 PM, "core-libs-dev on behalf of Roger Riggs" > > <core-libs-dev-boun...@openjdk.java.net on behalf of > > roger.ri...@oracle.com> wrote: > > > > Hi Clive, > > > > To clarify about the Oracle copyright, it should be included only if > > the > > file was > > derived from Oracle copyright source. The previous copyright is fine. > > > > Sorry for any confusion. > > > > Regards, Roger > > > > On 2/4/20 5:20 PM, Roger Riggs wrote: > > > Hi Clive, > > > > > > The update looks good to me. > > > > > > Thanks, Roger > > > > > > > > > On 2/4/20 5:17 PM, Verghese, Clive wrote: > > >> Hi Roger, > > >> > > >> Thank you for the feedback. I have addressed your comments and > > >> updated the Webrev. > > >> http://cr.openjdk.java.net/~alvdavi/webrevs/8235699/ > > >> > > >> Regards, > > >> Clive Verghese > > >> > > >> > > >> On 2/4/20, 12:34 PM, "core-libs-dev on behalf of Roger Riggs" > > >> <core-libs-dev-boun...@openjdk.java.net on behalf of > > >> roger.ri...@oracle.com> wrote: > > >> > > >> Hi Clive, > > >> A few comments: > > >> The copyrights for the new files need to follow the > > >> template in > > >> <repo>make/template/gpl-header. > > >> In particular, it needs to include Oracle as a copyright > > holder. > > >> bug235699.java: > > >> The @summary should be more informative; "it works" tells > > >> nothing about > > >> the case being tested. > > >> Please rename bug8235699.java to Bug8235699.java; > > >> There are more initial upper case tests than lower and it would > > >> be good > > >> to converge over time. > > >> 35: A comment saying that a AIOOBE should not occur would > > >> be helpful. > > >> CalendarBuilderTest.java: > > >> Should describe the condition that is being created. > > >> 27: "Test that CalendarBuilder.toString does not produce > > IOOBE" > > >> Are all of the assignments necessary to cause the bug? > > >> Remove any that are not; they are misleading. (onlysetting the > > >> year is > > >> needed) > > >> Thanks, Roger > > >> On 1/2/20 4:18 PM, Verghese, Clive wrote: > > >> > Hi Alan, > > >> > > > >> > Thanks for the feedback, > > >> > > > >> > I have removed the @Author tag and updated the tests as per > > >> your recommendation. > > >> > > > >> > Updated Webrev > > >> > http://cr.openjdk.java.net/~phh/8235699/webrev.04/ > > >> > > > >> > Regards, > > >> > Clive Verghsese > > >> > > > >> > Regards, > > >> > Clive Verghese > > >> > > > >> > On 1/2/20, 11:19 AM, "Volker Simonis" <simon...@amazon.de> > > wrote: > > >> > > > >> > On 02.01.20 18:39, Alan Bateman wrote: > > >> > > On 02/01/2020 13:26, Volker Simonis wrote: > > >> > >> : > > >> > >> > > >> http://cr.openjdk.java.net/~simonis/webrevs/2020/8235699.02/ > > >> > >> > > >> > >> Ready to push? > > >> > >> > > >> > > You shouldn't need to use core reflection here. > > Instead > > >> you can create > > >> > > the test so that it is compiled and runs as if part of > > >> the java.text > > >> > > package, e.g. > > >> > > > > >> > > @build java.base/java.text.CalendarBuilderToStringTest > > >> > > @main Driver > > >> > > > > >> > > > >> > Thanks for the hint. I wasn't aware of this possibility. > > >> > I think Clive will rewrite the test. > > >> > > > >> > > Do you really want the @author tag? We try to avoid > > >> them if possible > > >> > > because they are so hard to remove, even when > > >> code/tests are changed > > >> > > significantly. > > >> > > > >> > No not really. It was just a part of the template I > > used :) > > >> > @Clive: please feel free to remove the author tag. > > >> > > > >> > > -Alan > > >> > > > >> > > > >> > > > >> > > > >> > > > > > > > > > >
