Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug in List removeLast method #18

Closed
jafingerhut opened this issue Sep 12, 2019 · 2 comments
Closed

bug in List removeLast method #18

jafingerhut opened this issue Sep 12, 2019 · 2 comments

Comments

@jafingerhut
Copy link
Contributor

jafingerhut commented Sep 12, 2019

The value of n shown below is the smallest one I know of that exhibits this problem.

One issue with generative testing and data structures like List with wide branching factors is that it can take a large number of operations to reach "interesting" tree structures that exercise new code paths. I have found that making a local change with SHIFT_INCREMENT=2 and thus MAX_BRANCHES=(1 << 2) = 4 causes the existing generative tests to find issues like this in a fairly short period of time.

That requires a few replacements of literal constants 5 and 32 in the List.java and ListNodes.java source files to be replaced with appropriate names, to avoid replacing them manually in multiple places. I can submit a PR with such changes if that would be of interest.

$ clj -Sdeps '{:deps {io.lacuna/bifurcan {:mvn/version "0.2.0-alpha1"} org.clojure/clojure {:mvn/version "1.10.1"}}}'
(import '(io.lacuna.bifurcan List))
(def n 33824)
(def l1 (.concat (List.) (List/from (range n))))
(= (seq (range n)) (iterator-seq (.iterator l1)))
;; true.  Good
(.size l1)
;; 33824 - the expected size

(def l2 (.removeLast l1))
(= (seq (range (dec n))) (iterator-seq (.iterator l2)))
;; false.  bug!
(.size l2)
;; 1055 - not even close to the correct value
@jafingerhut
Copy link
Contributor Author

As a comparison point, with the changes of SHIFT_INCREMENT=2, the same test fails with n=84 (but no smaller).

jafingerhut added a commit to jafingerhut/bifurcan that referenced this issue Sep 12, 2019
This one: lacuna#18

At the very least, it does not cause existing tests to fail, and the
sequence of operations reported in the issue result in correct answers
with these changes, but not without.

If the root node has only one child after removing the last tree node,
then return a new root node that is the one that is its first child.
If any _other_ node that is not the root is in this scenario, do not
change the root node of the tree.
@jafingerhut
Copy link
Contributor Author

There is a deftest test case for this issue proposed in PR #20, as well as another similar test that exposes a similar problem in removeFirst.

ztellman added a commit that referenced this issue Dec 30, 2019
@ztellman ztellman closed this as completed Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants