It seems that the implementation is not that broken actually. One can still have the add semantic with insert() in 1.5.x by using the appropriate compare function and apr_skiplist_insert_compare(), that is, a function that does not return 0 when entries are equal, but rather <0 for LIFO or >0 for FIFO (and stability) semantics.
Add-if-not-exist semantic for apr_skiplist_insert() restored in r1666411, along with apr_skiplist_add() with the FIFO semantic. Will now backport to 1.6 and 1.5 branches, the latter without apr_skiplist_add() of course... On Fri, Mar 13, 2015 at 11:57 AM, Jim Jagielski <[email protected]> wrote: > >> >> Soooo.... if apr_snprintf("%d" were to, on every 50th int, print >> it out in decimal form, that would be a "design error"? :) >> >> That's a terrible analogy :) >> > > Yeah, I agree. > >> >> If so, how can we call it a "skiplist" which has a set of compliant >> expectations? It's not a skiplist implementation at all since it >> is broken. >> >> >> >> That's a useful point. FWIW I very quickly found the doc of an alternative >> skiplist implementation that says >> >> ---cut here--- >> You can enable duplicate keys by using the following: >> >> (sample code to create a skiplist via a non-default argument) >> >> This is an experimental feature. See the "KNOWN ISSUES" section below. >> ---cut here--- >> > > Looking over the original skiplist impl, which I assume is the > canon one, duplicates are allowed. Also, fwiw, the wikipedia entry > also specififically shows that dups are allowed. Plus, any > discussion on stability of the skiplist also describes handling > of dups. > >> I suppose there's no way to avoid different camps on this: a particular >> treatment of duplicates by the code that's been released is so basic that an >> app would likely be dependent on it vs. any callers anticipated that >> duplicates would be handled in a different way and we need to fix it. >> >> IMO here are the salient points: >> >> * There's no problem making a backwards-compatible, "compliant" >> implementation in the APR 1.6.x branch by adding more APIs. >> * There's no problem making APR trunk behave however is appropriate for the >> long term with no qualms about supporting existing code >> * httpd has the issue now, and most people that responded to my query on the >> httpd list thought it was a good idea for httpd to have its own copy that >> can more easily track httpd needs without adding any speed bumps to httpd >> users picking up the new level (i.e., upgrading APR, whether it is a new APR >> 1.5.x or 1.6.x); (I happen to think that's better for APR in the long term, >> but perhaps that's just me.) > > I am fine w/ APR doing whatever it wants. My point about > about httpd "suffering" due to APR, and APR suffering due > to the demands of httpd, are still valid I think.
