infinite loop in 'partition' and 'partition-all' when 'step' or 'n' is not positive

Question

If you pass a non-positive value of 'n' or 'step' to partition, you get an infinite loop. Here are a few examples:

(partition 0 (link: 1 2 3))
(partition 1 -1 (link: 1 2 3))

Cause: partition and partition-all do not check that n and step are positive.

Approach: Add checks to partition and partition-all that n and step are positive.

Patch: clj-1647_3.patch

Prescreened by: Alex Miller

Answer 1

Comment made by: alexmiller

Go for it

Answer 2

Comment made by: alexmiller

Go for it

Answer 3

Comment made by: alexmiller

Go for it

Answer 4

Comment made by: alexmiller

Go for it

Answer 5

Comment made by: alexmiller

Sorry, browser fail :)

Answer 6

Comment made by: kworam

Thanks for picking it up Matthew, I appreciate it!

Answer 7

Comment made by: kworam

Thanks for picking it up Matthew, I appreciate it!

Answer 8

_Comment made by: alex+import_

New patch: clj-1647.patch

Includes tests, fewer whitespace changes, manually thrown IAEs. I'll do some basic benchmarking soon, although I expect the overhead to be quite low as we're only checking the arguments once.

Kevin, the reason your patch was working for partition-all but not partition is that partition is defined early-ish in the bootstrapping process and the {:pre .. :post ..} maps aren't read by defn until it's enhanced later on.

Answer 9

Comment made by: kworam

Thanks for solving that mystery Matthew!

Answer 10

Comment made by: alexmiller

Patch looks basically good. Minor changes:

• internal-partition and internal-partition-all should be marked private with {{defn-}}.
• Commit description should start with "CLJ-1647"

Answer 11

Comment made by: mjg123

I added clj-1647_2.patch to supersede other patches on this issue. Jira ref is added to commit msg and defn- used where possible (defn- is not defined until after one of the private fns but there is the ^:private metadata added manually)

Answer 12

Comment made by: alexmiller

The patch changes add-annotation from defn- to defn but that seems unrelated to the intent of the patch?

Answer 13

Comment made by: mjg123

Thanks for looking so quickly Alex - sorry about that error in add-annotation. See clj-1647_3.patch

Answer 14

Reference: https://clojure.atlassian.net/browse/CLJ-1647 (reported by kworam)