On Thu, 20 Feb 2025 21:29:34 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Although it is not a checked exception, the fact that >> IllegalArgumentException is already documented on this method is considered >> to outweigh any benefit of a new Exception type such as >> IllegalStateException. And IllegalArgumentException is not that far off from >> existing usage. >> Existing usage includes the case that "the specified data is not appropriate >> to be set in this profile". > >> > Although, there's no time where such a modification will be allowed >> >> Exactly, it will throw exception every time a built-In profile is passed >> unlike IllegalStateException which is thrown only during an inappropriate >> time for instance when you are trying to add an element to the queue when it >> is already full. Plus setData() already throws IAE for other invalid >> argument cases. > > I see no contradiction here. Inappropriate time is a vague definition. For > built-in object, it is always inappropriate time to call `setData`. > > `IllegalArgumentException` implies that the argument is invalid, and if I > change the argument, the call will succeed — but the call will never succeed > for a built-in profile *because **the state of the object** doesn't allow it*. > > This is why `IllegalStateException` is more appropriate. > >> Although it is not a checked exception, the fact that >> IllegalArgumentException is already documented on this method is considered >> to outweigh any benefit of a new Exception type such as >> IllegalStateException. > > I do not agree here. The exception type should convey the reason it's thrown, > otherwise there wouldn't be so many types of exceptions. > > And two `@throws` with the same type aren't allowed in javadoc, which means > you have to add the new reason to throw `IllegalArgumentException` into the > existing one. Let's consider a scenario. If I call `setData` with `tagSignature` and `tagData` which cannot be interpreted correctly, I get `IllegalArgumentException`, which is reasonable — I passed invalid arguments. Then if I call `setData` with valid values for `tagSignature` and `tagData`, the call succeeds even with a built-in color profile. After this fix, this call would fail with `IllegalArgumentException` but I know that the arguments are **valid** because the call succeeded previously. And however I change the arguments, the call will never succeed. Throwing `IllegalStateException` will convey the updated behaviour in a cleaner way: it is *the state* of the object that makes the call to fail, not the arguments. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964372049
