On Sat, Mar 7, 2015 at 5:03 AM, Yann Ylavic <[email protected]> wrote:
> On Sat, Mar 7, 2015 at 8:30 AM, William A. Rowe Jr. <[email protected]> > wrote: > > On Thu, 5 Mar 2015 11:19:40 -0500 > > Jim Jagielski <[email protected]> wrote: > > > >> After doing some additional research, I think that the > >> current implementation of NOT allowing duplicates (in > >> APR 1.5/1.6) is BROKEN. In trunk this is determined by > >> a flag w/ the insert statement, but the default *should* > >> be to allow dups. > >> > >> As such, I'd like to adjust 1.5/1.6 to the correct and, > >> afaict, canonical behavior. > > > > I don't believe we can do this, it defies all rational API compatibility > > revisioning policies of any project, anywhere. > > There is a possibility to both preserve/restore the addn semantic to > skiplist_insert() and still have the add semantic in 1.5.x. > It would be to give apr_skiplist_insert_compare(), which exists in > 1.5.x, the add semantic when its compare function arg is NULL. > While it doesn't change the function signature, it still is an API change in that an application would require a certain minimum 1.5.x release for a an API "feature." IMO: * Add APIs as necessary to 1.6.x so that existing 1.5.x calls work as-is. (e.g., apr_skiplist_init_ex(..., new_parameters)) * Release 1.6.x soon Meanwhile, I would like to point out ("whine", "nag", whatever) that in my past, poor effort to bring the API documentation for this up from a relatively poor state, I didn't figure out everything that needed to be documented and I wasn't sure that the code was really complete. This is an appropriate point to stop and see what else is lurking. One way to approach it would be to work on improving the documentation, and seeing where that takes you in the code. (Easy to say... I probably don't have time myself, so I'm just throwing that out into the wind.) > > Eg. for trunk: > > Index: tables/apr_skiplist.c > =================================================================== > --- tables/apr_skiplist.c (revision 1664776) > +++ tables/apr_skiplist.c (working copy) > @@ -517,29 +517,39 @@ static apr_skiplistnode *insert_compare(apr_skipli > APR_DECLARE(apr_skiplistnode *) > apr_skiplist_insert_compare(apr_skiplist *sl, void *data, > apr_skiplist_compare comp) > { > - return insert_compare(sl, data, comp, 1); > + if (comp) { > + return insert_compare(sl, data, comp, 0); > + } > + else if (sl->compare) { > + return insert_compare(sl, data, sl->compare, 1); > + } > + else { > + return NULL; > + } > } > > APR_DECLARE(apr_skiplistnode *) apr_skiplist_insert(apr_skiplist *sl, > void *data) > { > - if (!sl->compare) { > - return NULL; > - } > - return insert_compare(sl, data, sl->compare, 1); > + return apr_skiplist_insert_compare(sl, data, sl->compare); > } > > -APR_DECLARE(apr_skiplistnode *) > apr_skiplist_addne_compare(apr_skiplist *sl, void *data, > +APR_DECLARE(apr_skiplistnode *) apr_skiplist_add_compare(apr_skiplist > *sl, void *data, > apr_skiplist_compare comp) > { > - return insert_compare(sl, data, comp, 0); > + if (comp) { > + return insert_compare(sl, data, comp, 1); > + } > + else if (sl->compare) { > + return insert_compare(sl, data, sl->compare, 1); > + } > + else { > + return NULL; > + } > } > > -APR_DECLARE(apr_skiplistnode *) apr_skiplist_addne(apr_skiplist *sl, > void *data) > +APR_DECLARE(apr_skiplistnode *) apr_skiplist_add(apr_skiplist *sl, void > *data) > { > - if (!sl->compare) { > - return NULL; > - } > - return insert_compare(sl, data, sl->compare, 0); > + return apr_skiplist_add_compare(sl, data, sl->compare); > } > > APR_DECLARE(int) apr_skiplist_remove(apr_skiplist *sl, void *data, > apr_skiplist_freefunc myfree) > -- > > > And for 1.5.x: > > Index: tables/apr_skiplist.c > =================================================================== > --- tables/apr_skiplist.c (revision 1664805) > +++ tables/apr_skiplist.c (working copy) > @@ -394,8 +394,16 @@ APR_DECLARE(apr_skiplistnode *) apr_skiplist_inser > apr_skiplist_compare comp) > { > apr_skiplistnode *m, *p, *tmp, *ret = NULL; > - int nh = 1, ch; > + int add = 0, nh = 1, ch; > > + if (!comp) { > + if (!sl->compare) { > + return NULL; > + } > + comp = sl->compare; > + add = 1; > + } > + > if (sl->preheight) { > while (nh < sl->preheight && get_b_rand()) { > nh++; > @@ -423,6 +431,11 @@ APR_DECLARE(apr_skiplistnode *) apr_skiplist_inser > */ > if (m->next) { > int compared = comp(data, m->next->data); > + if (compared == 0 && !add) { > + /* Keep the existing element(s) */ > + skiplist_qclear(&sl->stack_q); > + return NULL; > + } > if (compared > 0) { > m = m->next; > continue; > -- > -- Born in Roswell... married an alien... http://emptyhammock.com/
