On Wed, 10 Feb 2021 18:46:33 GMT, Phil Race <p...@openjdk.org> wrote:
>> From what I can tell, if clip is null, then calling clip(null) has no effect.
>> And it won't throw an NPE in that case. So the proposed documentation is
>> less accurate than no documentation at all.
>> And I would shy away from changing the implementation on this without LOTS
>> of VERY careful testing.
>> Oh the javadoc clause you propose is split by many lines of comments. I see
>> you have the rider "and clip is already set via {@code setClip}".
>> Well I am not sure why it has to be set by setClip for this to be true but
>> just having to say this sounds weird.
>
>> usrClip is set to null if "sh" is null so clip is cleared...I will update
>> the setClip doc too..
>
> Huh ? Not understanding you. If the userClip is non-null then we enter this
> if (usrClip != null) {
> s = intersectShapes(usrClip, s, true, true);
> }
> which i where you get the NPE from you are proposing to document.
It seems to me to be a little accidental from the implementation that
clip(null) will just silently return
if the user clip is currently null.
But I think it unduly risky to "clean that up" after so many years.
So I think we are stuck with some variant on the weird wording.
The bit about using setClip is bogus. I suggest adding the clause worded like
this
@throws NullPointerException if {@code s} is {@ code null) and a user clip is
currently set.
and explanatory text like this
Since this method intersects the specified shape with the current clip, it will
throw NPE for a null shape
unless the user clip is also null. So calling this method with a null argument
is not recommended.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2476
