[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(S) 8172288: Fix error message when trying to define an existing package to a module
From:       harold seigel <harold.seigel () oracle ! com>
Date:       2017-01-12 20:47:10
Message-ID: b4886952-2936-082f-50a6-ff56310d62bb () oracle ! com
[Download RAW message or body]

Thanks Karen!

Harold


On 1/12/2017 3:46 PM, Karen Kinnear wrote:
> Harold,
> 
> This looks much cleaner!
> 
> thanks,
> Karen
> 
> > On Jan 12, 2017, at 2:18 PM, harold seigel <harold.seigel@oracle.com> wrote:
> > 
> > Hi,
> > 
> > Please review this updated webrev: \
> > http://cr.openjdk.java.net/~hseigel/bug_8172288.hs.3/webrev/index.html 
> > It contains the three changes listed below.  Only file modules.cpp has changed \
> > since the last webrev. 
> > This fix was tested with the JTreg hotspot, java/io, java/lang, java/util and \
> > other JTReg tests, with the JCK lang and VM tests, and with RBT tiers2 - tier5 \
> > tests on Linux X64. 
> > Thanks, Harold
> > 
> > 
> > On 1/12/2017 10:16 AM, harold seigel wrote:
> > > Thanks everyone for all the reviews.
> > > 
> > > I'll post a new webrev with the following changes, and testing details, once \
> > > the re-testing completes. 
> > > 1. Rename throw_dup_pkg_IAE() to throw_dup_pkg_exception()
> > > 
> > > 2. Remove the package_name argument from throw_dup_pkg_exception() because the \
> > > package name can be extracted from the existing_pkg argument. 
> > > 3. Remove dupl_pk_index and use existing_pkg instead.
> > > 
> > > Harold
> > > 
> > > 
> > > On 1/12/2017 9:30 AM, George Triantafillou wrote:
> > > > Hi Karen,
> > > > 
> > > > On 1/12/2017 9:26 AM, Karen Kinnear wrote:
> > > > > Harold,
> > > > > 
> > > > > Looks good - with the same minor comment George caught, perhaps \
> > > > > throw_dup_pkg_exception? 
> > > > > George - there are two tests attached.
> > > > Thanks, I saw the additions to test/runtime/modules/JVMDefineModule.java, but \
> > > > I wanted to know which hotspot tests were run with Harold's change. 
> > > > -George
> > > > > thanks,
> > > > > Karen
> > > > > 
> > > > > > On Jan 12, 2017, at 9:18 AM, George Triantafillou \
> > > > > > <george.triantafillou@oracle.com> wrote: 
> > > > > > Hi Harold,
> > > > > > 
> > > > > > src/share/vm/classfile/modules.cpp - lines 266, 449, and 806:
> > > > > > 
> > > > > > Change "throw_dup_pkg_IAE" to "throw_dup_pkg_ISE".
> > > > > > 
> > > > > > How was the change tested?  Thanks.
> > > > > > 
> > > > > > -George
> > > > > > 
> > > > > > On 1/12/2017 8:50 AM, harold seigel wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > Please review this updated webrev for this bug.  The updated webrev \
> > > > > > > includes a small JDK change and hotspot changes to throw the correct \
> > > > > > > exceptions. 
> > > > > > > New JDK webrev: \
> > > > > > > http://cr.openjdk.java.net/~hseigel/bug_8172288.jdk.2/webrev/index.html \
> > > > > > >  New Hotspot webrev: \
> > > > > > > http://cr.openjdk.java.net/~hseigel/bug_8172288.hs.2/ 
> > > > > > > Note also that the bug has been re-titled to: Fix Jigsaw related \
> > > > > > > module/package error messages and throw correct exceptions 
> > > > > > > Thanks, Harold
> > > > > > > 
> > > > > > > On 1/10/2017 10:18 AM, harold seigel wrote:
> > > > > > > > Yes.  I'll modify the bug to include that.
> > > > > > > > 
> > > > > > > > What exception should the JVM throw for this?
> > > > > > > > 
> > > > > > > > Thanks, Harold
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 1/10/2017 10:12 AM, Alan Bateman wrote:
> > > > > > > > > On 10/01/2017 15:02, harold seigel wrote:
> > > > > > > > > 
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > Please review this small fix for JDK-8172288. Here's a sample of \
> > > > > > > > > > the new message: 
> > > > > > > > > > Package mypackage6 for module dupl.pkg.module is already in \
> > > > > > > > > > another module, Module_A, defined to the class loader 
> > > > > > > > > > Adding the class loader name to the message will be done as part \
> > > > > > > > > > of JDK-8169559 \
> > > > > > > > > > <https://bugs.openjdk.java.net/browse/JDK-8169559>. 
> > > > > > > > > > Open Webrev: \
> > > > > > > > > > http://cr.openjdk.java.net/~hseigel/bug_8172288/webrev/index.html
> > > > > > > > > IllegalArgumentException isn't a good exception for these cases, is \
> > > > > > > > > now the time to proposing this too? 
> > > > > > > > > -Alan


[prev in list] [next in list] [prev in thread] [next in thread]