Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/15/2018 05:26 PM, mandy chung wrote: > On 3/15/18 3:58 AM, Aleksey Shipilev wrote: >> On 03/15/2018 10:38 AM, David Holmes wrote: >>> To be pedantic, talking about "overriding static methods" is as wrong as >>> talking about "inheriting >>> static methods. They can't be inherited and so

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 3/15/18 3:58 AM, Aleksey Shipilev wrote: On 03/15/2018 10:38 AM, David Holmes wrote: To be pedantic, talking about "overriding static methods" is as wrong as talking about "inheriting static methods. They can't be inherited and so can't be overridden. In this context perhaps "intercept"

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/15/2018 09:57 AM, Aleksey Shipilev wrote: On 03/15/2018 08:56 AM, Peter Levart wrote: Hi Aleksey, In test, the following comment:   26  * @summary This is a test to ensure that proxies do not inherit static methods. I think the word "inherit" is not correct here. Interface static

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/15/2018 10:38 AM, David Holmes wrote: > On 15/03/2018 5:56 PM, Peter Levart wrote: >> Hi Aleksey, >> >> In test, the following comment: >> >>    26  * @summary This is a test to ensure that proxies do not inherit >> static methods. >> >> I think the word "inherit" is not correct here.

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 15/03/2018 5:56 PM, Peter Levart wrote: Hi Aleksey, In test, the following comment:   26  * @summary This is a test to ensure that proxies do not inherit static methods. I think the word "inherit" is not correct here. Interface static methods can not be inherited. VM already ensures

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/15/2018 08:56 AM, Peter Levart wrote: > Hi Aleksey, > > In test, the following comment: > >   26  * @summary This is a test to ensure that proxies do not inherit static > methods. > > I think the word "inherit" is not correct here. Interface static methods can > not be inherited. VM >

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

Hi Aleksey, In test, the following comment:   26  * @summary This is a test to ensure that proxies do not inherit static methods. I think the word "inherit" is not correct here. Interface static methods can not be inherited. VM already ensures that. Perhaps the comment should be:    

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/14/2018 10:44 PM, mandy chung wrote: > David - I think the test fails even in your first version. > > It should use ProxyClashTest.class.getClassLoader() to define the proxy class > as the test is running > in agent vm mode. Right. This passes local testing:

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On Wed, Mar 14, 2018 at 4:31 PM, Aleksey Shipilev wrote: > On 03/14/2018 07:09 PM, David Lloyd wrote: >> On Wed, Mar 14, 2018 at 1:00 PM, mandy chung wrote: >>> Thanks for adding the new test. Looks okay and some minor comment. >>> >>> +try {

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

David - I think the test fails even in your first version. It should use ProxyClashTest.class.getClassLoader() to define the proxy class as the test is running in agent vm mode. Mandy On 3/14/18 2:31 PM, Aleksey Shipilev wrote: Have you tried to run the test Because it fails: $ make images

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/14/2018 07:09 PM, David Lloyd wrote: > On Wed, Mar 14, 2018 at 1:00 PM, mandy chung wrote: >> Thanks for adding the new test. Looks okay and some minor comment. >> >> +try { >>: >> +} catch (Throwable e) { >> +

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/14/2018 07:59 PM, Andrej Golovnin wrote: > Hi David, > > +if (! Modifier.isStatic(m.getModifiers())) { > > I think the whitespace after the ‘!’-sign should be removed. I agree. No problem, I will remove this space in my patch queue. -Aleksey

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

Hi David, +if (! Modifier.isStatic(m.getModifiers())) { I think the whitespace after the ‘!’-sign should be removed. Best regards, Andrej Golovnin > On 14. Mar 2018, at 19:09, David Lloyd wrote: > > On Wed, Mar 14, 2018 at 1:00 PM, mandy chung

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/14/2018 07:44 PM, mandy chung wrote: I assume your colleague at Red Hat will sponsor it for you? >>> I will find out. >> I can do it, but I need to get updated on two things: >> a) Are we pushing JDK changes directly to jdk/jdk now? >> b) Do we need to run it through JDK Submit first,

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 3/14/18 11:37 AM, Aleksey Shipilev wrote: On 03/14/2018 07:09 PM, David Lloyd wrote: On Wed, Mar 14, 2018 at 1:00 PM, mandy chung wrote: Thanks for adding the new test. Looks okay and some minor comment. +try { : +} catch (Throwable e) { +

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 03/14/2018 07:09 PM, David Lloyd wrote: > On Wed, Mar 14, 2018 at 1:00 PM, mandy chung wrote: >> Thanks for adding the new test. Looks okay and some minor comment. >> >> +try { >>: >> +} catch (Throwable e) { >> +

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 3/14/18 11:09 AM, David Lloyd wrote: On Wed, Mar 14, 2018 at 1:00 PM, mandy chung wrote: Thanks for adding the new test. Looks okay and some minor comment. +try { : +} catch (Throwable e) { +System.err.println("\nTEST FAILED:"); +

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On Wed, Mar 14, 2018 at 1:00 PM, mandy chung wrote: > Thanks for adding the new test. Looks okay and some minor comment. > > +try { >: > +} catch (Throwable e) { > +System.err.println("\nTEST FAILED:"); > +e.printStackTrace();

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 3/13/18 5:16 PM, David Lloyd wrote: OK, done. It's a little bigger now so I'm attaching it. Thanks for adding the new test.   Looks okay and some minor comment. +try { : +} catch (Throwable e) { +System.err.println("\nTEST FAILED:"); +

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

Looks good -Sundar On 14/03/18, 5:49 AM, David Lloyd wrote: On Tue, Mar 13, 2018 at 7:16 PM, David Lloyd wrote: On Tue, Mar 13, 2018 at 6:31 PM, mandy chung wrote: I prefer to keep the original test case i.e. create a proxy class from

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On Tue, Mar 13, 2018 at 7:16 PM, David Lloyd wrote: > On Tue, Mar 13, 2018 at 6:31 PM, mandy chung wrote: >> I prefer to keep the original test case i.e. create a proxy class from >> Runnable and Observer. Instead add a new test case to create a

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On Tue, Mar 13, 2018 at 6:31 PM, mandy chung wrote: > I prefer to keep the original test case i.e. create a proxy class from > Runnable and Observer. Instead add a new test case to create a proxy class > with ClashWithRunnable, Runnable and Observer and verify that it

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

On 3/13/18 4:06 PM, David Lloyd wrote: I worked up a little patch for 8188240. I was able to co-opt an existing test which now fails before the patch and passes after. It's a tiny patch so I'm including it inline. I've CC'd Mandy because she filed the original bug. Here's the patch (use