Daniil,
Even as far back as 2007 there was concern that changing the current
behaviour might break existing code. That has to be an even bigger
concern now!
Further the spec is sloppy here:
" Once the eventRequest is deleted, no operations (for example,
EventRequest.setEnabled(boolean)) are permitted."
This is too loose. What is an "operation"? Is a query like isEnabled()
really an "operation"? I would not consider it so. And if we can delete
requests why is there no "isDeleted" query? The spec seems incomplete
and too vague.
To me this something that should have been clarified in the spec first
and then the implementation brought into alignment. But that should have
happened many years ago. Changing this now seems risky to me.
This change in long standing behaviour also requires a CSR request if it
is to proceed.
David
-----
On 30/03/2018 8:36 AM, Daniil Titov wrote:
Hi Serguei,
Please review a new version of the fix that has these places corrected.
Webreb: http://cr.openjdk.java.net/~dtitov/4613913/webrev.03
Bug: https://bugs.openjdk.java.net/browse/JDK-4613913
Thanks!
Best regards,
Daniil
On 3/29/18, 11:46 AM, "[email protected]"
<[email protected]> wrote:
Hi Daniil,
It looks good in general.
One minor comment is that it would be nice to make a cleanup
(as we already discussed) for all places like this:
202 if (isEnabled() || deleted) {
203 throw invalidState();
204 }
As the isEnabled() now checks for deleted and throws the invalidState()
then we can simplify these fragments to be:
202 if (isEnabled()) {
203 throw invalidState();
204 }
Thanks,
Serguei
On 3/29/18 10:27, Daniil Titov wrote:
> Please review the changes that ensure that no operation on deleted
com.sun.jdi.request.EventRequest objects are permitted as per JDI specification
for
com.sun.jdi.request.EventRequestManager.deleteEventRequest(com.sun.jdi.request.EventRequest)
method. The fix makes the following 4 methods in class com.sun.tools.jdi.
EventRequestManagerImpl$EventRequestImpl to throw
com.sun.jdi.request.InvalidRequestStateException if the request is deleted:
> - getProperty()
> - putProperty(Object, Object)
> - suspendPolicy()
> - isEnabled()
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-4613913
> Webrev: http://cr.openjdk.java.net/~dtitov/4613913/webrev.02/
>
> Best regards,
> Daniil
>
>
