[PATCH] improve javadoc in TreeSet#add api documentation
Kishor Gollapalliwar
kishor.gollapalliwar at gmail.com
Fri Nov 2 03:12:09 UTC 2018
Hello Stuart,
Thank you for providing this opportunity.
I'm up for this challenge and I'd love to take this task.
The only huddle would be, how to proceed, ie planning things step by step,
as this is my first time.
If you can help me with the planning, I'll do the rest and fix these issues.
Thank,
Kishor Gollapalliwar
On Fri 2 Nov, 2018, 07:27 Stuart Marks <stuart.marks at oracle.com wrote:
> 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