[PATCH] improve javadoc in TreeSet#add api documentation
Stuart Marks
stuart.marks at oracle.com
Fri Nov 2 01:57:10 UTC 2018
Hi Kishor,
This is indeed an issue with the documentation. The root of the issue is that
the implementations of the SortedSet and SortMap interfaces do not use the
equals() method to determine set or map membership. Instead they use a
comparison method (a Comparator, if provided, or the object's compareTo method,
if Comparable). This is a subtle semantic difference, which IMHO isn't
adequately explained in this section of the specification.
There has been some discussion of this before; see
https://bugs.openjdk.java.net/browse/JDK-8190545
Although that bug discusses the contains() method, the same semantic issue is
present in the add() method and in several other methods. The general issue is
explained in the SortedSet class doc, though it's rather buried in the
discussion about the concept of "consistent with equals".
This issue shows up in the add() and contains() method, and in other methods
such as remove() -- and possibly others -- because the text has been copied from
Set interface. (Probably erroneously.)
Instead of patching individual methods of TreeSet, I think a more comprehensive
approach is necessary. The SortedSet class doc should be clarified regarding use
of the comparison method for set membership, and it should provide method doc --
possibly by adding overrides -- for all the methods for which set membership is
significant. Then, implementations such as TreeSet should inherit the method
text from the SortedSet methods, using {@inheritDoc} if possible. I don't think
NavigableSet will be affected, but it should get a close look to make sure. The
j.u.c.ConcurrentSkipListSet docs will probably also need to be looked at.
(And all of the foregoing applies to SortMap and friends.)
This is rather larger than a simple doc change to a single method -- are you up
for it? :-)
s'marks
On 10/22/18 9:31 PM, Kishor Gollapalliwar wrote:
> Hello Martin and Everyone,
>
> @Martin : Thank you for the response.
>
> I have attached the PATCH file to this email.
>
> Request you to sponsor this change.
>
> Please find file extract in case my attachment stripped out.
>
> diff -r ebe635565ff3 src/share/classes/java/util/TreeSet.java
> --- a/src/share/classes/java/util/TreeSet.java Fri Oct 19 10:30:26 2018
> -0700
> +++ b/src/share/classes/java/util/TreeSet.java Tue Oct 23 09:44:44 2018
> +0530
> @@ -235,14 +235,19 @@
> }
>
> /**
> - * Adds the specified element to this set if it is not already present.
> - * More formally, adds the specified element {@code e} to this set if
> - * the set contains no element {@code e2} such that
> - * <tt>(e==null ? e2==null : e.equals(e2))</tt>.
> - * If this set already contains the element, the call leaves the set
> - * unchanged and returns {@code false}.
> + * Adds the specified {@code newElement} to this set, if and only if
> following conditions are met.
> + * <ol>
> + * <li>The {@code newElement} is <b>NOT {@code null}</b> (unless,
> {@link Comparator}
> + * which supports/permits {@code null} comparison was provided at set
> creation time.</li>
> + * <li><b>AND</b> there is no {@code setElement}, such that {@code
> newElement.compareTo(setElement)==0}
> + * ({@link Comparator#compare} {@code (newElement, setElement)==0},
> + * in case {@link Comparator} was provided at set creation time.)</li>
> + * </ol>
> + *
> + * <p>If any of above conditions are <b>NOT</b> met, it will be
> interpreted as
> + * set already contains the element, hence call leaves the set
> unchanged and returns {@code false}.</p>
> *
> - * @param e element to be added to this set
> + * @param newElement, the element to be added to this set
> * @return {@code true} if this set did not already contain the
> specified
> * element
> * @throws ClassCastException if the specified object cannot be
> compared
> @@ -251,8 +256,8 @@
> * and this set uses natural ordering, or its comparator
> * does not permit null elements
> */
> - public boolean add(E e) {
> - return m.put(e, PRESENT)==null;
> + public boolean add(E newElement) {
> + return m.put(newElement, PRESENT)==null;
> }
>
> /**
> Please revert in case of any queries.
>
> Thanks & Regards,
> *Kishor Golapelliwar,*
> The ability to convert ideas to things is the secret to outward success.
>
> On Mon, Oct 22, 2018 at 6:07 AM Martin Buchholz <martinrb at google.com> wrote:
>
>> Hi Kishor,
>>
>> I think your attachment was stripped out - standard policy for this
>> mailing list, I think.
>> For a small patch like this, you should just include the patch inline in
>> text form.
>>
>>
>> On Thu, Oct 18, 2018 at 2:21 AM, Kishor Gollapalliwar <
>> kishor.gollapalliwar at gmail.com> wrote:
>>
>>> Hi Team,
>>>
>>> Please provide your approval to update TreeSet#add method as it's
>>> documentation and behavior are not consistent.
>>>
>>> For more information, please refer my mail below and attachment.
>>>
>>> Thanks & Regards,
>>> *Kishor Golapelliwar,*
>>> The ability to convert ideas to things is the secret to outward success.
>>>
>>> ---------- Forwarded message ---------
>>> From: Kishor Gollapalliwar <kishor.gollapalliwar at gmail.com>
>>> Date: Mon, Oct 15, 2018 at 11:40 AM
>>> Subject: Re: Inconsistencies in TreeSet Interface
>>> To: <martinrb at google.com>
>>> Cc: <core-libs-dev at openjdk.java.net>
>>>
>>>
>>> Hello Everyone,
>>>
>>> This is the first time I am writing mail to this group, hence please
>>> ignore
>>> my nuisance and guide me towards right direction.
>>>
>>> As suggested by Martin, I believe documentation need to be updated.
>>> I would happy to update the documentation, please provide your inputs.
>>> I have attache the .java file for more details.
>>>
>>> @Martin : I really appreciate your advice and help, thank you.
>>>
>>> Thank you,
>>> Kishor Gollapalliwar
>>>
>>> On Sat, Oct 13, 2018 at 11:40 PM Martin Buchholz <martinrb at google.com>
>>> wrote:
>>>
>>>> core-libs-dev is the right mailing list.
>>>>
>>>> The documentation could be improved, but it's nuanced - consider using a
>>>> TreeSet with a Comparator that accepts null.
>>>>
>>>> On Sat, Oct 13, 2018 at 7:26 AM, Kishor Gollapalliwar <
>>>> kishor.gollapalliwar at gmail.com> wrote:
>>>>
>>>>> Hello Everyone,
>>>>>
>>>>> Introduction : I’m an enthusiast java developer. I’m a newbie in this
>>>>> group, hence please ignore my nuisance and guide me towards right
>>>>> direction.
>>>>>
>>>>> ## Problems Statement
>>>>>
>>>>> Treeset#add method documentations : “adds the specified element e to
>>> this
>>>>> set if the set contains no element e2 such that (e==null ? e2==null :
>>>>> e.equals(e2))”
>>>>>
>>>>> Inconsistencies:
>>>>>
>>>>>> If we try to add “null” value, it will throws NullPointerException,
>>>>> hence
>>>>>> e and e2 can not be null.
>>>>>
>>>>>
>>>>>
>>>>>> And e.compareTo(e2) ==0 || compare(e, e2)==0, it will not add to
>>>>>> collection.
>>>>>
>>>>>
>>>>> Request you to help me understand if behavior of Treeset#add method is
>>> as
>>>>> expected, and differ with set interface documentation. We may have to
>>>>> consider to update method documentations, if behavior is expected.
>>>>>
>>>>> For more details I have attached the sample java application.
>>>>>
>>>>> I’ve tested the behavior on Orack JDK 8u181 (64 bit) and Open JDK
>>> 8u181.
>>>>>
>>>>> Thanks & Regards,
>>>>> *Kishor Golapelliwar,*
>>>>> The ability to convert ideas to things is the secret to outward
>>> success.
>>>>>
>>>>
>>>>
>>>
>>
>>
More information about the core-libs-dev
mailing list