Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

David Holmes
Hi Chris,

Adding in hotspot-runtime-dev now that you have included the VM side of
the cleanup. What repo are you planning on pushing this to?

On 21/12/2017 9:45 AM, Chris Hegarty wrote:
>
>> On 20 Dec 2017, at 19:21, mandy chung <[hidden email]> wrote:
>>
>> The native side and hotspot side should also be cleaned up.

Thanks Mandy, I was about call that out too :)

> Good call. I think the following is probably as far as we want to
> go. Maybe a follow-on issue could be filed if deeper VM clean
> up is needed?
>
> http://cr.openjdk.java.net/~chegar/8179424/webrev.01/ <http://cr.openjdk.java.net/~chegar/8179424/webrev.01/>

src/hotspot/share/include/jvm.h

Can you fix an existing typo please:

!  * error if it is not marked propertly.

propertly -> properly

Also you seem to have missed this test reference:

./langtools/tools/jdeps/jdkinternals/src/p/Main.java:        Class<?>
caller = Reflection.getCallerClass(2);

Otherwise all changes seem fine.

Thanks,
David

> -Chris.
>
> P.S. jdk and hotspot tests are still running...
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Chris Hegarty
David,

> On 21 Dec 2017, at 00:42, David Holmes <[hidden email]> wrote:
>
> Hi Chris,
>
> Adding in hotspot-runtime-dev now that you have included the VM side of the cleanup. What repo are you planning on pushing this to?

Since there are now changes in the hotspot area, then it
probably makes sense to push this through jdk/hs.

> ...
>
> Can you fix an existing typo please:
> !  * error if it is not marked propertly.
> propertly -> properly

Fixed.

> Also you seem to have missed this test reference:
> ./langtools/tools/jdeps/jdkinternals/src/p/Main.java:        Class<?> caller = Reflection.getCallerClass(2);

Oops. Adding langtools testing too.

Updated webrev:
  http://cr.openjdk.java.net/~chegar/8179424/webrev.02/

What’s the correct set of tests and route into jdk/hs these
days? I’ve not pushed there in a while.

-Chris.

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

David Holmes
On 21/12/2017 7:20 PM, Chris Hegarty wrote:

> David,
>
>> On 21 Dec 2017, at 00:42, David Holmes <[hidden email]> wrote:
>>
>> Hi Chris,
>>
>> Adding in hotspot-runtime-dev now that you have included the VM side of the cleanup. What repo are you planning on pushing this to?
>
> Since there are now changes in the hotspot area, then it
> probably makes sense to push this through jdk/hs.

Okay.

>> ...
>>
>> Can you fix an existing typo please:
>> !  * error if it is not marked propertly.
>> propertly -> properly
>
> Fixed.
>
>> Also you seem to have missed this test reference:
>> ./langtools/tools/jdeps/jdkinternals/src/p/Main.java:        Class<?> caller = Reflection.getCallerClass(2);
>
> Oops. Adding langtools testing too.
>
> Updated webrev:
>    http://cr.openjdk.java.net/~chegar/8179424/webrev.02/

I don't quite follow the change to the langtools test. Is it just trying
to reference something in jdk.unsupported? I don't know what the "patch"
does.

> What’s the correct set of tests and route into jdk/hs these
> days? I’ve not pushed there in a while.

I'll email you direct.

Thanks,
David

> -Chris.
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Alan Bateman
On 21/12/2017 09:29, David Holmes wrote:
> :
>>
>> Updated webrev:
>>    http://cr.openjdk.java.net/~chegar/8179424/webrev.02/
>
> I don't quite follow the change to the langtools test. Is it just
> trying to reference something in jdk.unsupported? I don't know what
> the "patch" does.
I looked through webrev.02 and it looks okay. I assume there will be a
follow-up bug created to re-examine JVM_GetCallerClass.

The update to the jdeps test does look a bit odd, wouldn't it be better
to change it to another internal API?

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Chris Hegarty
David, Alan,

> On 21 Dec 2017, at 09:54, Alan Bateman <[hidden email]> wrote:
>
> On 21/12/2017 09:29, David Holmes wrote:
>> :
>>>
>>> Updated webrev:
>>>    http://cr.openjdk.java.net/~chegar/8179424/webrev.02/
>>
>> I don't quite follow the change to the langtools test. Is it just trying to reference something in jdk.unsupported? I don't know what the "patch" does.
> I looked through webrev.02 and it looks okay. I assume there will be a follow-up bug created to re-examine JVM_GetCallerClass.
>
> The update to the jdeps test does look a bit odd, wouldn't it be better to change it to another internal API?

The test is about identifying StackWalker as the replacement
supported API for getCallerClass, which is continues to do.
I could add yet another scenario to test for a different internal
API that also has a replacement, and add the appropriate
@modules to the test to expose its package.

-Chris.

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

David Holmes
On 21/12/2017 8:20 PM, Chris Hegarty wrote:

> David, Alan,
>
>> On 21 Dec 2017, at 09:54, Alan Bateman <[hidden email]> wrote:
>>
>> On 21/12/2017 09:29, David Holmes wrote:
>>> :
>>>>
>>>> Updated webrev:
>>>>     http://cr.openjdk.java.net/~chegar/8179424/webrev.02/
>>>
>>> I don't quite follow the change to the langtools test. Is it just trying to reference something in jdk.unsupported? I don't know what the "patch" does.
>> I looked through webrev.02 and it looks okay. I assume there will be a follow-up bug created to re-examine JVM_GetCallerClass.
>>
>> The update to the jdeps test does look a bit odd, wouldn't it be better to change it to another internal API?
>
> The test is about identifying StackWalker as the replacement
> supported API for getCallerClass, which is continues to do.

Won't pretend I actually understand that, but as long as the test works
reliably then okay.

Thanks,
David

> I could add yet another scenario to test for a different internal
> API that also has a replacement, and add the appropriate
> @modules to the test to expose its package.
>
> -Chris.
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Chris Hegarty

> On 21 Dec 2017, at 12:33, David Holmes <[hidden email]> wrote:
> ...
>> The test is about identifying StackWalker as the replacement
>> supported API for getCallerClass, which is continues to do.
>
> Won't pretend I actually understand that, but as long as the test works reliably then okay.

Jdeps prints helpful messages when it encounters usage of certain
common internal APIs, e.g. "I see your are using
sun.reflect.Reflection.getCallerClass(int). This is an internal API which
has been replaced by java.lang.StackWalker. You may wanna update
your code to use that API instead”.

For example,
 http://hg.openjdk.java.net/jdk/jdk/file/tip/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/resources/jdkinternals.properties#l16

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Mandy Chung
In reply to this post by Chris Hegarty


On 12/21/17 2:20 AM, Chris Hegarty wrote:

> David, Alan,
>
>> On 21 Dec 2017, at 09:54, Alan Bateman <[hidden email]> wrote:
>>
>> On 21/12/2017 09:29, David Holmes wrote:
>>> :
>>>> Updated webrev:
>>>>     http://cr.openjdk.java.net/~chegar/8179424/webrev.02/
>>> I don't quite follow the change to the langtools test. Is it just trying to reference something in jdk.unsupported? I don't know what the "patch" does.
>> I looked through webrev.02 and it looks okay. I assume there will be a follow-up bug created to re-examine JVM_GetCallerClass.
>>
>> The update to the jdeps test does look a bit odd, wouldn't it be better to change it to another internal API?
> The test is about identifying StackWalker as the replacement
> supported API for getCallerClass, which is continues to do.
> I could add yet another scenario to test for a different internal
> API that also has a replacement, and add the appropriate
> @modules to the test to expose its package.

The test shows sun.reflect.Reflection as a removed API seems odd since
the class is present but not getCallerClass(int).

p.Main is used to check that reference to sun.reflect.Reflection is
flagged as JDK internal use and not a removed class.  I suggest to
change it to use another sun.reflect.Reflection API and create an issue
to follow up sun.reflect.Reflection as flagged as a removed API.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Chris Hegarty

> On 21 Dec 2017, at 16:09, mandy chung <[hidden email]> wrote:
>
>>> ...
>> The test is about identifying StackWalker as the replacement
>> supported API for getCallerClass, which is continues to do.
>> I could add yet another scenario to test for a different internal
>> API that also has a replacement, and add the appropriate
>> @modules to the test to expose its package.
>>
>
> The test shows sun.reflect.Reflection as a removed API seems odd since the class is present but not getCallerClass(int).

There appears to be some confusion here. My webrev REMOVES
sun.reflect.Reflection completely, since getCallerClass(int) was its
last method. For compilation purposes, the test uses a patched
jdk.unsupported module with sun.reflect.Reflection, but that class
is not present at runtime. So the sun.reflect.Reflection internal API
has been removed, no?

> p.Main is used to check that reference to sun.reflect.Reflection is flagged as JDK internal use and not a removed class.  I suggest to change it to use another sun.reflect.Reflection API

There is no other API in sun.reflect.Reflection, unless you mean
to use something in sun.reflect.ReflectionFactory ?

> and create an issue to follow up sun.reflect.Reflection as flagged as a removed API.

That’s what the test now does with my changes. Why separate it
out into a separate issue?   If we need a test for an internal API, I
can add a scenario that uses sun.reflect.ReflectionFactory.

-Chris.

Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Mandy Chung


On 12/21/17 9:18 AM, Chris Hegarty wrote:

>> On 21 Dec 2017, at 16:09, mandy chung <[hidden email]> wrote:
>>
>>>> ...
>>> The test is about identifying StackWalker as the replacement
>>> supported API for getCallerClass, which is continues to do.
>>> I could add yet another scenario to test for a different internal
>>> API that also has a replacement, and add the appropriate
>>> @modules to the test to expose its package.
>>>
>> The test shows sun.reflect.Reflection as a removed API seems odd since the class is present but not getCallerClass(int).
> There appears to be some confusion here. My webrev REMOVES
> sun.reflect.Reflection completely, since getCallerClass(int) was its
> last method. For compilation purposes, the test uses a patched
> jdk.unsupported module with sun.reflect.Reflection, but that class
> is not present at runtime. So the sun.reflect.Reflection internal API
> has been removed, no?

That clarifies it.  Thanks.
>
>> p.Main is used to check that reference to sun.reflect.Reflection is flagged as JDK internal use and not a removed class.  I suggest to change it to use another sun.reflect.Reflection API
> There is no other API in sun.reflect.Reflection, unless you mean
> to use something in sun.reflect.ReflectionFactory ?
>

ReflectionFactory or other class in sun.reflect package would do it.
>> and create an issue to follow up sun.reflect.Reflection as flagged as a removed API.
> That’s what the test now does with my changes. Why separate it
> out into a separate issue?   If we need a test for an internal API, I
> can add a scenario that uses sun.reflect.ReflectionFactory.

Yes please. That's what this case intends to verify.

Thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Chris Hegarty

On 21/12/17 21:40, mandy chung wrote:
>  ... >
> ReflectionFactory or other class in sun.reflect package would do it.
>>> and create an issue to follow up sun.reflect.Reflection as flagged as a removed API.
>> That’s what the test now does with my changes. Why separate it
>> out into a separate issue?   If we need a test for an internal API, I
>> can add a scenario that uses sun.reflect.ReflectionFactory.
>
> Yes please. That's what this case intends to verify.

Webrev with the test updated as suggested:
   http://cr.openjdk.java.net/~chegar/8179424/webrev.03/

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Mandy Chung


On 12/22/17 2:16 AM, Chris Hegarty wrote:
>
> Webrev with the test updated as suggested:
>   http://cr.openjdk.java.net/~chegar/8179424/webrev.03/
>

Looks good.  Thanks for updating the test.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

Alan Bateman
In reply to this post by Chris Hegarty
On 22/12/2017 10:16, Chris Hegarty wrote:

>
> On 21/12/17 21:40, mandy chung wrote:
>>  ... >
>> ReflectionFactory or other class in sun.reflect package would do it.
>>>> and create an issue to follow up sun.reflect.Reflection as flagged
>>>> as a removed API.
>>> That’s what the test now does with my changes. Why separate it
>>> out into a separate issue?   If we need a test for an internal API, I
>>> can add a scenario that uses sun.reflect.ReflectionFactory.
>>
>> Yes please. That's what this case intends to verify.
>
> Webrev with the test updated as suggested:
>   http://cr.openjdk.java.net/~chegar/8179424/webrev.03/
Updated webrev looks good to me too.

-Alan