Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v6]

2022-10-19 Thread Nir Lisker 
On Wed, 19 Oct 2022 12:51:21 GMT, Douglas Held  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().~~
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> Douglas Held has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update Point3D.java
>   
>   Per review by nlisker

I can keep it.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Kevin Rushforth 
On Wed, 19 Oct 2022 12:52:33 GMT, Nir Lisker  wrote:

> It's not. I have mentioned this exact case in 
> https://bugs.openjdk.org/browse/JDK-8226930. I can assign it to you (or 
> self-assign it yourself) if you want to work on it. If not, I will get to it 
> at some point (no pun intended). This issue will require to go over the 
> classes that override equals and see if they do it properly, including 
> `hashcode`. Sometimes the `equals` implementation is "good enough", so 
> there's no need to rewrite it in all the classes, but especially for public 
> APIs, users should expect correct implementations.

And it's not just a doc issue: Point3D (at least) violates the contract of 
`equals` and `hashCode`, since two objects that are considered `equals` can 
produce different hash codes (e.g., the case of two points, one of which has a 
value of `+0.0` for one of the coords, and the other of which has `-0.0` for 
the same coord). Depending on how we change it, it might need a CSR.

So I'd prefer that either you keep the bug or @arapte take it.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v6]

2022-10-19 Thread Douglas Held 
On Wed, 19 Oct 2022 12:51:21 GMT, Douglas Held  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().~~
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> Douglas Held has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update Point3D.java
>   
>   Per review by nlisker

I wouldn't want to take on this fix, as I'm not well educated in floating point 
types. I'm glad it is an identified issue.

--
Douglas Held
Senior Principal Security Consultant
NetSuite Product Security Team
***@***.***

Note: Sent from a phone/microphone which may have introduced errors I didn’t 
catch

On 19 Oct 2022, at 13:52, nlisker ***@***.***> wrote:



I think there's something else of quality worth discussing here. Is == 
appropriate given the values under comparison are double?

It's not. I have mentioned this exact case in 
https://bugs.openjdk.org/browse/JDK-8226930. I can assign it to you (or 
self-assign it yourself) if you want to work on it. If not, I will get to it at 
some point (no pun intended). This issue will require to go over the classes 
that override equals and see if they do it properly, including hashcode. 
Sometimes the equals implementation is "good enough", so there's not need to 
rewrite it in all the classes, but especially for public APIs, users should 
expect correct implementations.

—
Reply to this email directly, view it on 
GitHub,
 or 
unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Nir Lisker 
On Wed, 19 Oct 2022 12:46:03 GMT, Douglas Held  wrote:

> I think there's something else of quality worth discussing here. Is `==` 
> appropriate given the values under comparison are `double`?

It's not. I have mentioned this exact case in 
https://bugs.openjdk.org/browse/JDK-8226930. I can assign it to you (or 
self-assign it yourself) if you want to work on it. If not, I will get to it at 
some point (no pun intended). This issue will require to go over the classes 
that override equals and see if they do it properly, including `hashcode`. 
Sometimes the `equals` implementation is "good enough", so there's not need to 
rewrite it in all the classes, but especially for public APIs, users should 
expect correct implementations.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Nir Lisker 
On Wed, 19 Oct 2022 12:38:54 GMT, Douglas Held  wrote:

>> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 417:
>> 
>>> 415:  * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
>>> 416:  *
>>> 417:  * @param {@code obj} the reference object with which to compare
>> 
>> Don't use `@code` for the parameter declaration, they are treated 
>> automatically by the javadoc tool.
>
> Don't use it for `obj` you mean?

Here, yes. The parameter name following `@param` is taken care of automatically 
(a hyphen is also added after it).

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]

2022-10-19 Thread Nir Lisker 
On Wed, 19 Oct 2022 12:33:37 GMT, Douglas Held  wrote:

> I thought the code tags produced hyperlinks.

Those would be the `@link` tags, or `@linkplain`.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Douglas Held 
On Wed, 19 Oct 2022 12:36:30 GMT, Nir Lisker  wrote:

>> Douglas Held has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update Point3D.java
>>   
>>   Per review
>
> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 417:
> 
>> 415:  * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
>> 416:  *
>> 417:  * @param {@code obj} the reference object with which to compare
> 
> Don't use `@code` for the parameter declaration, they are treated 
> automatically by the javadoc tool.

Don't use it for `obj` you mean?

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Douglas Held 
On Wed, 19 Oct 2022 12:41:20 GMT, Nir Lisker  wrote:

>> Don't use it for `obj` you mean?
>
> Here, yes. The parameter name following `@param` is taken care of 
> automatically (a hyphen is also added after it).

and we don't need period `.` at the end of each `param` or `returns` line?

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Douglas Held 
On Wed, 19 Oct 2022 12:34:15 GMT, Douglas Held  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().~~
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> Douglas Held has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update Point3D.java
>   
>   Per review

I think there's something else of quality worth discussing here. Is `==` 
appropriate given the values under comparison are `double`?

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v6]

2022-10-19 Thread Douglas Held 
> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().~~
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

Douglas Held has updated the pull request incrementally with one additional 
commit since the last revision:

  Update Point3D.java
  
  Per review by nlisker

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/913/files
  - new: https://git.openjdk.org/jfx/pull/913/files/374f51e5..323b2f81

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=913=05
 - incr: https://webrevs.openjdk.org/?repo=jfx=913=04-05

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jfx/pull/913.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/913/head:pull/913

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Nir Lisker 
On Wed, 19 Oct 2022 12:34:15 GMT, Douglas Held  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().~~
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> Douglas Held has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update Point3D.java
>   
>   Per review

modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 417:

> 415:  * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
> 416:  *
> 417:  * @param {@code obj} the reference object with which to compare

Don't use `@code` for the parameter declaration, they are treated automatically 
by the javadoc tool.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]

2022-10-19 Thread Douglas Held 
On Tue, 18 Oct 2022 22:15:35 GMT, Douglas Held  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().~~
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> Douglas Held has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update Point3D.java
>   
>   Per review

I think this equals() method JavaDoc should be compliant now.

I must apologise. I thought the code tags produced hyperlinks. Now I can see 
they only apply a fixed width type font. Of course, the formatting tags need to 
apply in all of the places discussed.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v5]

2022-10-19 Thread Douglas Held 
> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().~~
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

Douglas Held has updated the pull request incrementally with one additional 
commit since the last revision:

  Update Point3D.java
  
  Per review

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/913/files
  - new: https://git.openjdk.org/jfx/pull/913/files/eb08b561..374f51e5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=913=04
 - incr: https://webrevs.openjdk.org/?repo=jfx=913=03-04

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jfx/pull/913.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/913/head:pull/913

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]

2022-10-18 Thread Nir Lisker 
On Tue, 18 Oct 2022 22:15:35 GMT, Douglas Held  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().~~
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> Douglas Held has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update Point3D.java
>   
>   Per review

modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 414:

> 412: /**
> 413:  * Indicates whether some other object is "equal to" this one.
> 414:  * Two instances of Point3D are equal if the return values of their

`Point3D` also needs `@code`.

modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 418:

> 416:  *
> 417:  * @param obj the reference object with which to compare
> 418:  * @return true if this Point3D is the same as the obj argument; 
> false otherwise

`Point3D`, `true`, `false` and `obj` need `@code`.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]

2022-10-18 Thread Douglas Held 
On Tue, 18 Oct 2022 11:32:04 GMT, Nir Lisker  wrote:

>> Agreed, mentioning equality criteria sounds good to me too. How does this 
>> look ?
>> 
>> 
>> /**
>>  * Indicates whether some other object is "equal to" this one.
>>  * Two instances of Point3D are equal if the return values of their
>>  * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
>>  *
>>  * @param obj the reference object with which to compare
>>  * @return true if this Point3D is the same as the obj argument; false 
>> otherwise
>>  */
>
> This is the current proposal by Douglas and align with `Object::equals` which 
> it is overriding. I'm fine with this wording. Just need to add the `@code` 
> tags.

I have added a commit to match the proposed block by arapte.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]

2022-10-18 Thread Douglas Held 
On Thu, 13 Oct 2022 14:16:23 GMT, Nir Lisker  wrote:

>> Douglas Held has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update Point3D.java
>>   
>>   Per review
>
> Changes requested by nlisker (Reviewer).

Tagged @nlisker to validate the code tags are included appropriately.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]

2022-10-18 Thread Douglas Held 
> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().~~
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

Douglas Held has updated the pull request incrementally with one additional 
commit since the last revision:

  Update Point3D.java
  
  Per review

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/913/files
  - new: https://git.openjdk.org/jfx/pull/913/files/0c52ce57..eb08b561

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=913=03
 - incr: https://webrevs.openjdk.org/?repo=jfx=913=02-03

  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jfx/pull/913.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/913/head:pull/913

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]

2022-10-18 Thread Nir Lisker 
On Tue, 18 Oct 2022 04:47:38 GMT, Ambarish Rapte  wrote:

>> I think that we should explain what makes 2 objects equal. Even if we don't 
>> explicitly name the methods used for comparison, we could say "2 points are 
>> equals if their coordinates are equal".
>> 
>> By the way, I have https://bugs.openjdk.org/browse/JDK-8226930 assigned to 
>> go over some dubious equals/hashcode implementations, in case you want to 
>> delegate the task.
>
> Agreed, mentioning equality criteria sounds good to me too. How does this 
> look ?
> 
> 
> /**
>  * Indicates whether some other object is "equal to" this one.
>  * Two instances of Point3D are equal if the return values of their
>  * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
>  *
>  * @param obj the reference object with which to compare
>  * @return true if this Point3D is the same as the obj argument; false 
> otherwise
>  */

This is the current proposal by Douglas and align with `Object::equals` which 
it is overriding. I'm fine with this wording. Just need to add the `@code` tags.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]

2022-10-17 Thread Ambarish Rapte 
On Mon, 17 Oct 2022 17:07:40 GMT, Nir Lisker  wrote:

>> It would be suitable to align with our existing doc comment in other 
>> classes, for example as here,
>> 
>> 1. 
>> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Point2D.java#L374
>> 2. 
>> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java#L103
>> 3. 
>> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/BoundingBox.java#L169
>> 
>> 
>> I would recommend to use as in Point2D,
>> 
>> 
>> /**
>>  * Indicates whether some other object is "equal to" this one.
>>  *
>>  * @param obj the reference object with which to compare
>>  * @return true if this Point3D is the same as the obj argument; false 
>> otherwise
>>  */
>
> I think that we should explain what makes 2 objects equal. Even if we don't 
> explicitly name the methods used for comparison, we could say "2 points are 
> equals if their coordinates are equal".
> 
> By the way, I have https://bugs.openjdk.org/browse/JDK-8226930 assigned to go 
> over some dubious equals/hashcode implementations, in case you want to 
> delegate the task.

Agreed, mentioning equality criteria sounds good to me too. How does this look ?


/**
 * Indicates whether some other object is "equal to" this one.
 * Two instances of Point3D are equal if the return values of their
 * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
 *
 * @param obj the reference object with which to compare
 * @return true if this Point3D is the same as the obj argument; false 
otherwise
 */

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]

2022-10-17 Thread Nir Lisker 
On Mon, 17 Oct 2022 15:24:48 GMT, Ambarish Rapte  wrote:

>> ...correction, found in toString() as well.
>
> It would be suitable to align with our existing doc comment in other classes, 
> for example as here,
> 
> 1. 
> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Point2D.java#L374
> 2. 
> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java#L103
> 3. 
> https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/BoundingBox.java#L169
> 
> 
> I would recommend to use as in Point2D,
> 
> 
> /**
>  * Indicates whether some other object is "equal to" this one.
>  *
>  * @param obj the reference object with which to compare
>  * @return true if this Point3D is the same as the obj argument; false 
> otherwise
>  */

I think that we should explain what makes 2 objects equal. Even if we don't 
explicitly name the methods used for comparison, we could say "2 points are 
equals if their coordinates are equal".

By the way, I have https://bugs.openjdk.org/browse/JDK-8226930 assigned to go 
over some dubious equals/hashcode implementations, in case you want to delegate 
the task.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]

2022-10-17 Thread Nir Lisker 
On Fri, 14 Oct 2022 15:08:24 GMT, Douglas Held  wrote:

>> Not strictly enforced, but adding a blank line does aid readability (of the 
>> source code...the generated docs don't care).
>
> If I were to add empty lines before JavaDoc tags such as @param etc, then for 
> consistency I think I should do it for the whole class, where they are not 
> present.
> 
> Note, on line 32: **// PENDING_DOC_REVIEW of this whole class**
> Maybe we should first decide whether we are scrubbing all doc for the whole 
> class, or making a minimal change to the obviously incorrect information. My 
> feeling is now tending toward the latter.
> 
> It could very well be that I misunderstood the comment by nlisker.

We just tend to separate the words section from the tags section with an empty 
line in the source code. You don't have to do it for the whole class (unless 
you want to), I just suggest to do it here since you're making a change anyway.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]

2022-10-17 Thread Ambarish Rapte 
On Fri, 14 Oct 2022 15:15:27 GMT, Douglas Held  wrote:

>> Looking at the JavaDoc in the rest of the class, the {@code Point3D} tags 
>> are not used outside of these two methods hashCode() and equals(). I'd 
>> propose actually that we remove them, for consistency with the rest of the 
>> JavaDoc in this class.
>
> ...correction, found in toString() as well.

It would be suitable to align with our existing doc comment in other classes, 
for example as here,

1. 
https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Point2D.java#L374
2. 
https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java#L103
3. 
https://github.com/openjdk/jfx/blob/9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d/modules/javafx.graphics/src/main/java/javafx/geometry/BoundingBox.java#L169


I would recommend to use as in Point2D,


/**
 * Indicates whether some other object is "equal to" this one.
 *
 * @param obj the reference object with which to compare
 * @return true if this Point3D is the same as the obj argument; false 
otherwise
 */

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]

2022-10-14 Thread Douglas Held 
On Fri, 14 Oct 2022 15:30:10 GMT, Douglas Held  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> Douglas Held has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update Point3D.java
>   
>   Removal of two line-end whitespace characters

I've reverted some of my changes which fell outside of the egregious 
documentation error.

If we are to correct further doc style, then that might be addressed as a 
whole-file review of the doc. (per the comment on line 32)

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]

2022-10-14 Thread Douglas Held 
> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

Douglas Held has updated the pull request incrementally with one additional 
commit since the last revision:

  Update Point3D.java
  
  Removal of two line-end whitespace characters

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/913/files
  - new: https://git.openjdk.org/jfx/pull/913/files/36b4dfb6..0c52ce57

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=913=02
 - incr: https://webrevs.openjdk.org/?repo=jfx=913=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jfx/pull/913.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/913/head:pull/913

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v2]

2022-10-14 Thread Douglas Held 
> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

Douglas Held has updated the pull request incrementally with one additional 
commit since the last revision:

  Update Point3D.java
  
  Undoing changes outside of equals() method.
  Accepting some but not all review comments in documentation for equals()

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/913/files
  - new: https://git.openjdk.org/jfx/pull/913/files/d7dff6be..36b4dfb6

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=913=01
 - incr: https://webrevs.openjdk.org/?repo=jfx=913=00-01

  Stats: 8 lines in 1 file changed: 1 ins; 2 del; 5 mod
  Patch: https://git.openjdk.org/jfx/pull/913.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/913/head:pull/913

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-14 Thread Douglas Held 
On Thu, 13 Oct 2022 14:15:58 GMT, Nir Lisker  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 417:
> 
>> 415:  * @param obj an object to be compared with this {@code Point3D}.
>> 416:  * @return true if the object to be compared is an instance of 
>> Point3D and
>> 417:  * has the same values; false otherwise.
> 
> `true`, `Point3D` and `false` should be in `{@code}`.

Looking at the JavaDoc in the rest of the class, the {@code Point3D} tags are 
not used outside of these two methods hashCode() and equals(). I'd propose 
actually that we remove them, for consistency with the rest of the JavaDoc in 
this class.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-14 Thread Douglas Held 
On Fri, 14 Oct 2022 15:13:41 GMT, Douglas Held  wrote:

>> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 417:
>> 
>>> 415:  * @param obj an object to be compared with this {@code Point3D}.
>>> 416:  * @return true if the object to be compared is an instance of 
>>> Point3D and
>>> 417:  * has the same values; false otherwise.
>> 
>> `true`, `Point3D` and `false` should be in `{@code}`.
>
> Looking at the JavaDoc in the rest of the class, the {@code Point3D} tags are 
> not used outside of these two methods hashCode() and equals(). I'd propose 
> actually that we remove them, for consistency with the rest of the JavaDoc in 
> this class.

...correction, found in toString() as well.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-14 Thread Douglas Held 
On Fri, 14 Oct 2022 15:03:41 GMT, Douglas Held  wrote:

>> I think this one is OK either as "if" or "if and only if" (if you do change 
>> it, I recommend spelling it out, although "iff" with no punctuation, would 
>> be acceptable). I agree with the request to use code style.
>
> I disagree with both suggestions. 
> 
> 1. I think the extra verbosity (while factually correct) adds nothing, for 
> such a straightforward concept as equality of points in 3 space.
> 2. x, y and z are private and are not exposed in the JavaDoc. Further, the 
> exposed methods getX() etc. that I **could** link to, are not actually what 
> are evaluated in the method body.
> 
> I'd propose alternately that we keep "x, y and z" but call them "coordinates" 
> and not "properties". This agrees with other parts of the JavaDoc such as the 
> constructor, and getX() etc. method documentation.

Correction: getX() etc. are used in the method. I will put the methods into 
code tags.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-14 Thread Douglas Held 
On Thu, 13 Oct 2022 15:50:38 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 415:
>> 
>>> 413:  * Determines whether or not two objects are equal. Two instances 
>>> of {@code Point3D}
>>> 414:  * are equal if the values of their x, y, and z properties are 
>>> equal.
>>> 415:  * @param obj an object to be compared with this {@code Point3D}.
>> 
>> We tend to put a new line before the tags to separate them from the 
>> description, not sure if it's enforced.
>
> Not strictly enforced, but adding a blank line does aid readability (of the 
> source code...the generated docs don't care).

If I were to add empty lines before JavaDoc tags such as @param etc, then for 
consistency I think I should do it for the whole class, where they are not 
present.

Note, on line 32: **// PENDING_DOC_REVIEW of this whole class**
Maybe we should first decide whether we are scrubbing all doc for the whole 
class, or making a minimal change to the obviously incorrect information. My 
feeling is now tending toward the latter.

It could very well be that I misunderstood the comment by nlisker.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-14 Thread Douglas Held 
On Thu, 13 Oct 2022 15:49:58 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 414:
>> 
>>> 412: /**
>>> 413:  * Determines whether or not two objects are equal. Two instances 
>>> of {@code Point3D}
>>> 414:  * are equal if the values of their x, y, and z properties are 
>>> equal.
>> 
>> I would even write "i.f.f" because it's bidirectional.
>> 
>> Also, `x`, `y`, and `z` should be in `{@code}`.
>
> I think this one is OK either as "if" or "if and only if" (if you do change 
> it, I recommend spelling it out, although "iff" with no punctuation, would be 
> acceptable). I agree with the request to use code style.

I disagree with both suggestions. 

1. I think the extra verbosity (while factually correct) adds nothing, for such 
a straightforward concept as equality of points in 3 space.
2. x, y and z are private and are not exposed in the JavaDoc. Further, the 
exposed methods getX() etc. that I **could** link to, are not actually what are 
evaluated in the method body.

I'd propose alternately that we keep "x, y and z" but call them "coordinates" 
and not "properties". This agrees with other parts of the JavaDoc such as the 
constructor, and getX() etc. method documentation.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-14 Thread Douglas Held 
On Thu, 13 Oct 2022 14:13:27 GMT, Nir Lisker  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 413:
> 
>> 411: 
>> 412: /**
>> 413:  * Determines whether or not two objects are equal. Two instances 
>> of {@code Point3D}
> 
> Since this object is known, I would start with "Determines if this point is 
> equals to a given object".

Agree. I propose the very similar wording "Determines whether this Point3D is 
equal to a given object."

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-13 Thread Kevin Rushforth 
On Thu, 13 Oct 2022 14:14:44 GMT, Nir Lisker  wrote:

>> The JavaDoc for equals had a copy/paste error. I normalized the text based 
>> on the JavaDoc for method java.awt.Point#equals. I also changed formatting 
>> in the method signatures of equals(), hashCode() and toString().
>> 
>> For good measure, some kind of copy/paste detection should probably be added 
>> to the many automated checks. For the entire OpenJDK project.
>
> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 414:
> 
>> 412: /**
>> 413:  * Determines whether or not two objects are equal. Two instances 
>> of {@code Point3D}
>> 414:  * are equal if the values of their x, y, and z properties are 
>> equal.
> 
> I would even write "i.f.f" because it's bidirectional.
> 
> Also, `x`, `y`, and `z` should be in `{@code}`.

I think this one is OK either as "if" or "if and only if" (if you do change it, 
I recommend spelling it out, although "iff" with no punctuation, would be 
acceptable). I agree with the request to use code style.

> modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 415:
> 
>> 413:  * Determines whether or not two objects are equal. Two instances 
>> of {@code Point3D}
>> 414:  * are equal if the values of their x, y, and z properties are 
>> equal.
>> 415:  * @param obj an object to be compared with this {@code Point3D}.
> 
> We tend to put a new line before the tags to separate them from the 
> description, not sure if it's enforced.

Not strictly enforced, but adding a blank line does aid readability (of the 
source code...the generated docs don't care).

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-13 Thread Kevin Rushforth 
On Wed, 12 Oct 2022 17:28:42 GMT, Douglas Held  wrote:

> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

A couple comments.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-13 Thread Nir Lisker 
On Wed, 12 Oct 2022 17:28:42 GMT, Douglas Held  wrote:

> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

Changes requested by nlisker (Reviewer).

modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 413:

> 411: 
> 412: /**
> 413:  * Determines whether or not two objects are equal. Two instances of 
> {@code Point3D}

Since this object is known, I would start with "Determines if this point is 
equals to a given object".

modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 414:

> 412: /**
> 413:  * Determines whether or not two objects are equal. Two instances of 
> {@code Point3D}
> 414:  * are equal if the values of their x, y, and z properties are equal.

I would even write "i.f.f" because it's bidirectional.

Also, `x`, `y`, and `z` should be in `{@code}`.

modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 415:

> 413:  * Determines whether or not two objects are equal. Two instances of 
> {@code Point3D}
> 414:  * are equal if the values of their x, y, and z properties are equal.
> 415:  * @param obj an object to be compared with this {@code Point3D}.

We tend to put a new line before the tags to separate them from the 
description, not sure if it's enforced.

modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 417:

> 415:  * @param obj an object to be compared with this {@code Point3D}.
> 416:  * @return true if the object to be compared is an instance of 
> Point3D and
> 417:  * has the same values; false otherwise.

`true`, `Point3D` and `false` should be in `{@code}`.

-

PR: https://git.openjdk.org/jfx/pull/913

Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D

2022-10-13 Thread Kevin Rushforth 
On Wed, 12 Oct 2022 17:28:42 GMT, Douglas Held  wrote:

> The JavaDoc for equals had a copy/paste error. I normalized the text based on 
> the JavaDoc for method java.awt.Point#equals. I also changed formatting in 
> the method signatures of equals(), hashCode() and toString().
> 
> For good measure, some kind of copy/paste detection should probably be added 
> to the many automated checks. For the entire OpenJDK project.

This is now available in JBS as 
[JDK-8295236](https://bugs.openjdk.org/browse/JDK-8295236). Once you update the 
title of this PR ~~and fix your whitespace errors~~, it will be made `rfr`.

Looks fine to me, but I'll let @arapte review and sponsor this.

-

PR: https://git.openjdk.org/jfx/pull/913