On Tue, 4 Mar 2025 12:44:11 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> The silently do nothing option was considered, but if you do that, then you >> have no easy way of knowing if it worked. >> Tests may pass spuriously, or fail later for the wrong reasons. So a worse >> choice. >> And in all my searching of uses of this API it is (1) tests in the JDK >> itself and (2) a couple of libraries that are targeting specific known >> profiles with issues and are fixed up - so never applied to built-in >> profiles. > >> can we just ignore it instead and did not use suspicion >> IllegalArgumentException for correct parameters? or change the type to >> something unrelated to "..ArgumentException"? > > This has been [my > suggestion](https://github.com/openjdk/jdk/pull/23606#discussion_r1968073992) > since the start of this code review. > >> The silently do nothing option was considered, but if you do that, then you >> have no easy way of knowing if it worked. >> Tests may pass spuriously, or fail later for the wrong reasons. So a worse >> choice. > > I agree, an exception should be thrown if modification isn't allowed. > >> And in all my searching of uses of this API it is (1) tests in the JDK >> itself and (2) a couple of libraries that are targeting specific known >> profiles with issues and are fixed up - so never applied to built-in >> profiles. > > Therefore we can throw <code>Illegal<i>State</i>Exception</code> instead of > <code>Illegal<i>Argument</i>Exception</code>. > > Quoting here [Harshitha's > reply](https://github.com/openjdk/jdk/pull/23606#discussion_r1968263836): > >> As Phil mentioned earlier, the already documented exception IAE for >> .setData() outweighs introducing a new exception. > > Using the exception which clearly describes the reason why it was thrown > outweighs re-using an already documented exception… > >> Using an existing exception would mean less disruption to any existing >> applications using the Java API. > > …especially because the search for usages of `setData` revealed that there > are no usages of `setData` which will be affected by the proposed change. > > If I'm not mistaken, the search also showed that no usages of `setData` > handle `IllegalArgumentException` — thus apps will be equally unprepared to > any type of an unchecked exception. @aivanov-jdk @mrserb There are two other exceptions that may be better suited that ISE in this case - [UnsupportedOperationException ](https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/lang/UnsupportedOperationException.html) and [ProfileDataException](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/java/awt/color/ProfileDataException.html) but the main motivation for going with IAE is that it is existing and documented exception. > If I'm not mistaken, the search also showed that no usages of setData handle > IllegalArgumentException — thus apps will be equally unprepared to any type > of an unchecked exception. Reasonable point. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1982200255
