On Thu, 20 Feb 2025 21:36:06 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>>> > 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. > 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. That's demonstrably untrue. This method already has two. Regarding the scenario above, this apparent and has all been considered already and the IAE is the preferred solution. And FWIW I don't think ISE is really any better. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964393430
