On Fri, 24 Jan 2025 20:31:01 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> (C)
>
> src/java.management/share/classes/com/sun/jmx/mbeanserver/PerInterface.java
> line 149:
>
>> 147: throws MBeanException, ReflectionException {
>> 148:
>> 149: // Construct the exception that we will throw
>
> Although your changes look fine, the existing code for constructing this
> exception is odd in that it artificially introduces a `cause` exception. It
> seems to mostly want to capture additional msg information, but most of it
> seems to be duplicated in the msg passed in, and all this could instead just
> be handled with a single msg constructed at the call site (and the
> ReflectionException could be allocated at the call site). Also, what it the
> @SuppressWarnings("removal") for?
Thanks -
Ah, "removal" was added for SM removal, that can surely go now. Actually I
don't see anything in what we're removing which required it, seems to have been
unnecessary.
The method is almost trivial now, and only used in two places. Yes it is a bit
odd to manufacture a new cause, rather than throw something immediately.
Elsewhere in this file, e.g. the getAttribute method does just throw new
AttributeNotFoundException(msg),
but here in invoke() we use "return noSuchMethod(...)" which effectively does:
throw new ReflectionException(new NoSuchMethodException(operation +
sigString(signature)), msg);
(Hah, yes unlike a new Exception(String message, Throwable cause),
ReflectionException uses new ReflectionException(Exception e, String message).)
Two lines like that mean that the noSuchMethod method can be removed. I'm
trying that out...
But changing the behaviour (throwing a different Exception) might come with
some compatibilty risk. I think it's good not to change the behaviour in this
property removal change.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23132#discussion_r1929381074
