From 7ba46c03a670152f18982eeabc8b38ccb3163c40 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 12:26:11 -0800 Subject: [PATCH 1/9] fix #13496 handle tombstones --- lib/pure/collections/tables.nim | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/pure/collections/tables.nim b/lib/pure/collections/tables.nim index 2e3adc6fb6c8a..1b6e7d6873ae7 100644 --- a/lib/pure/collections/tables.nim +++ b/lib/pure/collections/tables.nim @@ -1118,7 +1118,7 @@ iterator pairs*[A, B](t: TableRef[A, B]): (A, B) = ## # value: [1, 5, 7, 9] let L = len(t) for h in 0 .. high(t.data): - if isFilled(t.data[h].hcode): + if isFilledAndValid(t.data[h].hcode): yield (t.data[h].key, t.data[h].val) assert(len(t) == L, "the length of the table changed while iterating over it") @@ -1140,7 +1140,7 @@ iterator mpairs*[A, B](t: TableRef[A, B]): (A, var B) = let L = len(t) for h in 0 .. high(t.data): - if isFilled(t.data[h].hcode): + if isFilledAndValid(t.data[h].hcode): yield (t.data[h].key, t.data[h].val) assert(len(t) == L, "the length of the table changed while iterating over it") @@ -1182,7 +1182,7 @@ iterator values*[A, B](t: TableRef[A, B]): B = let L = len(t) for h in 0 .. high(t.data): - if isFilled(t.data[h].hcode): + if isFilledAndValid(t.data[h].hcode): yield t.data[h].val assert(len(t) == L, "the length of the table changed while iterating over it") @@ -1203,7 +1203,7 @@ iterator mvalues*[A, B](t: TableRef[A, B]): var B = let L = len(t) for h in 0 .. high(t.data): - if isFilled(t.data[h].hcode): + if isFilledAndValid(t.data[h].hcode): yield t.data[h].val assert(len(t) == L, "the length of the table changed while iterating over it") From 2ae5f3bc2418a15c0f7b1b19e07351b1002d191d Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 13:06:51 -0800 Subject: [PATCH 2/9] add test --- lib/pure/collections/tables.nim | 2 +- tests/collections/ttables.nim | 26 ++++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/lib/pure/collections/tables.nim b/lib/pure/collections/tables.nim index 1b6e7d6873ae7..cbe7578447ef9 100644 --- a/lib/pure/collections/tables.nim +++ b/lib/pure/collections/tables.nim @@ -1161,7 +1161,7 @@ iterator keys*[A, B](t: TableRef[A, B]): A = let L = len(t) for h in 0 .. high(t.data): - if isFilled(t.data[h].hcode): + if isFilledAndValid(t.data[h].hcode): yield t.data[h].key assert(len(t) == L, "the length of the table changed while iterating over it") diff --git a/tests/collections/ttables.nim b/tests/collections/ttables.nim index bba95c1f13fd2..b5441bea67443 100644 --- a/tests/collections/ttables.nim +++ b/tests/collections/ttables.nim @@ -173,9 +173,14 @@ block tableconstr: block ttables2: proc TestHashIntInt() = var tab = initTable[int,int]() - for i in 1..1_000_000: + when defined(nimTestsTablesDisableSlow): + # helps every single time when this test needs to be debugged + let n = 1_000 + else: + let n = 1_000_000 + for i in 1..n: tab[i] = i - for i in 1..1_000_000: + for i in 1..n: var x = tab[i] if x != i : echo "not found ", i @@ -395,3 +400,20 @@ block tablesref: orderedTableSortTest() echo "3" + +block: # https://github.com/nim-lang/Nim/issues/13496 + let t = newTable[int, int]() + t[15] = 1 + t[19] = 2 + t[17] = 3 + t[150] = 4 + doAssert t.len == 4 + t.del(150) + doAssert t.len == 3 + doAssert sortedItems(t.values) == @[1, 2, 3] + doAssert sortedItems(t.keys) == @[15, 17, 19] + doAssert sortedPairs(t) == @[(15, 1), (17, 3), (19, 2)] + var s = newSeq[int]() + for v in t.values: s.add(v) + assert s.len == 3 + doAssert sortedItems(s) == @[1, 2, 3] From a0865e525358494f3c36ae166d3a2ed0795722d0 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 14:20:33 -0800 Subject: [PATCH 3/9] more tests --- tests/collections/ttables.nim | 49 ++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/tests/collections/ttables.nim b/tests/collections/ttables.nim index b5441bea67443..9b7506d1a33cd 100644 --- a/tests/collections/ttables.nim +++ b/tests/collections/ttables.nim @@ -402,18 +402,37 @@ block tablesref: block: # https://github.com/nim-lang/Nim/issues/13496 - let t = newTable[int, int]() - t[15] = 1 - t[19] = 2 - t[17] = 3 - t[150] = 4 - doAssert t.len == 4 - t.del(150) - doAssert t.len == 3 - doAssert sortedItems(t.values) == @[1, 2, 3] - doAssert sortedItems(t.keys) == @[15, 17, 19] - doAssert sortedPairs(t) == @[(15, 1), (17, 3), (19, 2)] - var s = newSeq[int]() - for v in t.values: s.add(v) - assert s.len == 3 - doAssert sortedItems(s) == @[1, 2, 3] + template testDel(body) = + block: + body + when t is CountTable|CountTableRef: + t.inc(15, 1) + t.inc(19, 2) + t.inc(17, 3) + t.inc(150, 4) + t.del(150) + else: + t[15] = 1 + t[19] = 2 + t[17] = 3 + t[150] = 4 + t.del(150) + doAssert t.len == 3 + doAssert sortedItems(t.values) == @[1, 2, 3] + doAssert sortedItems(t.keys) == @[15, 17, 19] + doAssert sortedPairs(t) == @[(15, 1), (17, 3), (19, 2)] + var s = newSeq[int]() + for v in t.values: s.add(v) + assert s.len == 3 + doAssert sortedItems(s) == @[1, 2, 3] + when t is OrderedTable|OrderedTableRef: + doAssert toSeq(t.keys) == @[15, 19, 17] + doAssert toSeq(t.values) == @[1,2,3] + doAssert toSeq(t.pairs) == @[(15, 1), (19, 2), (17, 3)] + + testDel(): (var t: Table[int, int]) + testDel(): (let t = newTable[int, int]()) + testDel(): (var t: OrderedTable[int, int]) + testDel(): (let t = newOrderedTable[int, int]()) + testDel(): (var t: CountTable[int]) + testDel(): (let t = newCountTable[int]()) From 829e29353bce4429fef112e947c2d0386bd5e3de Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 14:33:44 -0800 Subject: [PATCH 4/9] remove deprecated procs --- tests/sets/tsets_various.nim | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/sets/tsets_various.nim b/tests/sets/tsets_various.nim index 8a63763b4e96b..72a22cf6bbc39 100644 --- a/tests/sets/tsets_various.nim +++ b/tests/sets/tsets_various.nim @@ -9,7 +9,7 @@ import sets, hashes block tsetpop: - var a = initSet[int]() + var a = initHashSet[int]() for i in 1..1000: a.incl(i) doAssert len(a) == 1000 @@ -50,7 +50,7 @@ block tsets2: "80"] block tableTest1: - var t = initSet[tuple[x, y: int]]() + var t = initHashSet[tuple[x, y: int]]() t.incl((0,0)) t.incl((1,0)) assert(not t.containsOrIncl((0,1))) @@ -63,7 +63,7 @@ block tsets2: # "{(x: 0, y: 0), (x: 0, y: 1), (x: 1, y: 0), (x: 1, y: 1)}") block setTest2: - var t = initSet[string]() + var t = initHashSet[string]() t.incl("test") t.incl("111") t.incl("123") @@ -102,9 +102,9 @@ block tsets2: block tsets3: let - s1: HashSet[int] = toSet([1, 2, 4, 8, 16]) - s2: HashSet[int] = toSet([1, 2, 3, 5, 8]) - s3: HashSet[int] = toSet([3, 5, 7]) + s1: HashSet[int] = toHashSet([1, 2, 4, 8, 16]) + s2: HashSet[int] = toHashSet([1, 2, 3, 5, 8]) + s3: HashSet[int] = toHashSet([3, 5, 7]) block union: let @@ -172,7 +172,7 @@ block tsets3: assert i in s1_s3 xor i in s1 assert i in s2_s3 xor i in s2 - assert((s3 -+- s3) == initSet[int]()) + assert((s3 -+- s3) == initHashSet[int]()) assert((s3 -+- s1) == s1_s3) block difference: @@ -191,7 +191,7 @@ block tsets3: for i in s2: assert i in s2_s3 xor i in s3 - assert((s2 - s2) == initSet[int]()) + assert((s2 - s2) == initHashSet[int]()) block disjoint: assert(not disjoint(s1, s2)) From 7282c2cf404781eb29345046a1c19e606e682746 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 14:42:52 -0800 Subject: [PATCH 5/9] _ --- tests/sets/tsets_various.nim | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/sets/tsets_various.nim b/tests/sets/tsets_various.nim index 72a22cf6bbc39..ce7433527b207 100644 --- a/tests/sets/tsets_various.nim +++ b/tests/sets/tsets_various.nim @@ -198,3 +198,26 @@ block tsets3: assert disjoint(s1, s3) assert(not disjoint(s2, s3)) assert(not disjoint(s2, s2)) + + +block: # https://github.com/nim-lang/Nim/issues/13496 + template testDel(body) = + block: + body + t.incl(15) + t.incl(19) + t.incl(17) + t.incl(150) + t.del(150) + doAssert t.len == 3 + doAssert sortedItems(t) == @[15, 17, 19] + var s = newSeq[int]() + for v in t.values: s.add(v) + assert s.len == 3 + doAssert sortedItems(s) == @[1, 2, 3] + when t is OrderedSet: + doAssert sortedPairs(t) == @[(15, 1), (17, 3), (19, 2)] + doAssert toSeq(t) == @[15, 19, 17] + + testDel(): (var t: HashSet[int]) + testDel(): (var t: OrderedSet[int]) From d7a89d438a2defa5ba2e3c51ef6e005ea9b01aa9 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 15:17:17 -0800 Subject: [PATCH 6/9] _ --- tests/sets/tsets_various.nim | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/sets/tsets_various.nim b/tests/sets/tsets_various.nim index ce7433527b207..4fd602510cd97 100644 --- a/tests/sets/tsets_various.nim +++ b/tests/sets/tsets_various.nim @@ -7,6 +7,11 @@ set is empty import sets, hashes +from sequtils import toSeq +from algorithm import sorted + +proc sortedPairs[T](t: T): auto = toSeq(t.pairs).sorted +template sortedItems(t: untyped): untyped = sorted(toSeq(t)) block tsetpop: var a = initHashSet[int]() @@ -199,7 +204,6 @@ block tsets3: assert(not disjoint(s2, s3)) assert(not disjoint(s2, s2)) - block: # https://github.com/nim-lang/Nim/issues/13496 template testDel(body) = block: @@ -208,15 +212,15 @@ block: # https://github.com/nim-lang/Nim/issues/13496 t.incl(19) t.incl(17) t.incl(150) - t.del(150) + t.excl(150) doAssert t.len == 3 doAssert sortedItems(t) == @[15, 17, 19] var s = newSeq[int]() - for v in t.values: s.add(v) + for v in t: s.add(v) assert s.len == 3 - doAssert sortedItems(s) == @[1, 2, 3] + doAssert sortedItems(s) == @[15, 17, 19] when t is OrderedSet: - doAssert sortedPairs(t) == @[(15, 1), (17, 3), (19, 2)] + doAssert sortedPairs(t) == @[(a: 0, b: 15), (a: 1, b: 19), (a: 2, b: 17)] doAssert toSeq(t) == @[15, 19, 17] testDel(): (var t: HashSet[int]) From 65f3b53a56869848aa85c467bfd2cb0a5ebd9c5b Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 18:39:54 -0800 Subject: [PATCH 7/9] fix #13504; add SharedTable tests --- lib/pure/collections/setimpl.nim | 6 +- lib/pure/collections/sharedtables.nim | 5 ++ lib/pure/collections/tableimpl.nim | 22 +++++-- lib/pure/collections/tables.nim | 4 ++ tests/sets/tsets_various.nim | 29 +++++++++ tests/stdlib/tsharedtable.nim | 86 +++++++++++++++++++++++++-- 6 files changed, 137 insertions(+), 15 deletions(-) diff --git a/lib/pure/collections/setimpl.nim b/lib/pure/collections/setimpl.nim index f8950e3546b3e..f7a48ab911013 100644 --- a/lib/pure/collections/setimpl.nim +++ b/lib/pure/collections/setimpl.nim @@ -38,7 +38,7 @@ proc enlarge[A](s: var HashSet[A]) = newSeq(n, len(s.data) * growthFactor) swap(s.data, n) # n is now old seq for i in countup(0, high(n)): - if isFilled(n[i].hcode): + if isFilledAndValid(n[i].hcode): var j = -1 - rawGetKnownHC(s, n[i].key, n[i].hcode) rawInsert(s, s.data, n[i].key, n[i].hcode, j) @@ -112,7 +112,7 @@ proc enlarge[A](s: var OrderedSet[A]) = swap(s.data, n) while h >= 0: var nxt = n[h].next - if isFilled(n[h].hcode): + if isFilled(n[h].hcode): # should be isFilledAndValid once tombstones are used var j = -1 - rawGetKnownHC(s, n[h].key, n[h].hcode) rawInsert(s, s.data, n[h].key, n[h].hcode, j) h = nxt @@ -130,7 +130,7 @@ proc exclImpl[A](s: var OrderedSet[A], key: A): bool {.inline.} = result = true while h >= 0: var nxt = n[h].next - if isFilled(n[h].hcode): + if isFilled(n[h].hcode): # should be isFilledAndValid once tombstones are used if n[h].hcode == hc and n[h].key == key: dec s.counter result = false diff --git a/lib/pure/collections/sharedtables.nim b/lib/pure/collections/sharedtables.nim index 0fbbdb3eb0e31..27ac5e84f7e3b 100644 --- a/lib/pure/collections/sharedtables.nim +++ b/lib/pure/collections/sharedtables.nim @@ -206,6 +206,11 @@ proc del*[A, B](t: var SharedTable[A, B], key: A) = withLock t: delImpl() +proc len*[A, B](t: var SharedTable[A, B]): int = + ## number of elements in `t` + withLock t: + result = t.counter + proc init*[A, B](t: var SharedTable[A, B], initialSize = 64) = ## creates a new hash table that is empty. ## diff --git a/lib/pure/collections/tableimpl.nim b/lib/pure/collections/tableimpl.nim index aabaeeeb37968..d7facda7209aa 100644 --- a/lib/pure/collections/tableimpl.nim +++ b/lib/pure/collections/tableimpl.nim @@ -107,13 +107,23 @@ template clearImpl() {.dirty.} = t.data[i].val = default(type(t.data[i].val)) t.counter = 0 +template ctAnd(a, b): bool = + # pending https://github.com/nim-lang/Nim/issues/13502 + when a: + when b: true + else: false + else: false + template initImpl(result: typed, size: int) = - assert isPowerOfTwo(size) - result.counter = 0 - newSeq(result.data, size) - when compiles(result.first): - result.first = -1 - result.last = -1 + when ctAnd(declared(SharedTable), type(result) is SharedTable): + init(result, size) + else: + assert isPowerOfTwo(size) + result.counter = 0 + newSeq(result.data, size) + when compiles(result.first): + result.first = -1 + result.last = -1 template insertImpl() = # for CountTable checkIfInitialized() diff --git a/lib/pure/collections/tables.nim b/lib/pure/collections/tables.nim index cbe7578447ef9..131804a2241c3 100644 --- a/lib/pure/collections/tables.nim +++ b/lib/pure/collections/tables.nim @@ -1282,6 +1282,10 @@ template forAllOrderedPairs(yieldStmt: untyped) {.dirty.} = var h = t.first while h >= 0: var nxt = t.data[h].next + # For OrderedTable/OrderedTableRef, isFilled is ok because `del` is O(n) + # and doesn't create tombsones, but if it does start using tombstones, + # carefully replace `isFilled` by `isFilledAndValid` as appropriate for these + # table types only, ditto with `OrderedSet`. if isFilled(t.data[h].hcode): yieldStmt h = nxt diff --git a/tests/sets/tsets_various.nim b/tests/sets/tsets_various.nim index 4fd602510cd97..c27d8e124caae 100644 --- a/tests/sets/tsets_various.nim +++ b/tests/sets/tsets_various.nim @@ -225,3 +225,32 @@ block: # https://github.com/nim-lang/Nim/issues/13496 testDel(): (var t: HashSet[int]) testDel(): (var t: OrderedSet[int]) + +block: # test correctness after a number of inserts/deletes + template testDel(body) = + block: + body + var expected: seq[int] + let n = 100 + let n2 = n*2 + for i in 0..=n or i mod 3 != 0) and i mod 7 != 0: + expected.add i + + for i in expected: doAssert i in t + doAssert t.len == expected.len + doAssert sortedItems(t) == expected + + testDel(): (var t: HashSet[int]) + testDel(): (var t: OrderedSet[int]) diff --git a/tests/stdlib/tsharedtable.nim b/tests/stdlib/tsharedtable.nim index 99d20e08a38cd..ce6aa96df84d2 100644 --- a/tests/stdlib/tsharedtable.nim +++ b/tests/stdlib/tsharedtable.nim @@ -6,10 +6,84 @@ output: ''' import sharedtables -var table: SharedTable[int, int] +block: + var table: SharedTable[int, int] -init(table) -table[1] = 10 -assert table.mget(1) == 10 -assert table.mgetOrPut(3, 7) == 7 -assert table.mgetOrPut(3, 99) == 7 + init(table) + table[1] = 10 + assert table.mget(1) == 10 + assert table.mgetOrPut(3, 7) == 7 + assert table.mgetOrPut(3, 99) == 7 + deinitSharedTable(table) + +import sequtils, algorithm +proc sortedPairs[T](t: T): auto = toSeq(t.pairs).sorted +template sortedItems(t: untyped): untyped = sorted(toSeq(t)) + +import tables # refs issue #13504 + +block: # we use Table as groundtruth, it's well tested elsewhere + template testDel(t, t0) = + template put2(i) = + t[i] = i + t0[i] = i + + template add2(i, val) = + t.add(i, val) + t0.add(i, val) + + template del2(i) = + t.del(i) + t0.del(i) + + template checkEquals() = + doAssert t.len == t0.len + for k,v in t0: + doAssert t.mgetOrPut(k, -1) == v # sanity check + doAssert t.mget(k) == v + + let n = 100 + let n2 = n*2 + let n3 = n*3 + let n4 = n*4 + let n5 = n*5 + + for i in 0.. Date: Tue, 25 Feb 2020 19:28:20 -0800 Subject: [PATCH 8/9] fix #https://github.com/nim-lang/Nim/issues/13505 intsets.missingOrExcl silently gave wrong results sometimes --- lib/pure/collections/intsets.nim | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/pure/collections/intsets.nim b/lib/pure/collections/intsets.nim index 7ca3797835a39..1967a01498d0d 100644 --- a/lib/pure/collections/intsets.nim +++ b/lib/pure/collections/intsets.nim @@ -330,6 +330,15 @@ proc excl*(s: var IntSet, other: IntSet) = for item in other: excl(s, item) +proc len*(s: IntSet): int {.inline.} = + ## Returns the number of elements in `s`. + if s.elems < s.a.len: + result = s.elems + else: + result = 0 + for _ in s: + inc(result) + proc missingOrExcl*(s: var IntSet, key: int): bool = ## Excludes `key` in the set `s` and tells if `key` was already missing from `s`. ## @@ -348,9 +357,9 @@ proc missingOrExcl*(s: var IntSet, key: int): bool = assert a.missingOrExcl(5) == false assert a.missingOrExcl(5) == true - var count = s.elems + var count = s.len exclImpl(s, key) - result = count == s.elems + result = count == s.len proc clear*(result: var IntSet) = ## Clears the IntSet back to an empty state. @@ -514,15 +523,6 @@ proc disjoint*(s1, s2: IntSet): bool = return false return true -proc len*(s: IntSet): int {.inline.} = - ## Returns the number of elements in `s`. - if s.elems < s.a.len: - result = s.elems - else: - result = 0 - for _ in s: - inc(result) - proc card*(s: IntSet): int {.inline.} = ## Alias for `len() <#len,IntSet>`_. result = s.len() From cde6d7c39fa760aa6b973136c8ef950cfdf8087e Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 25 Feb 2020 19:33:23 -0800 Subject: [PATCH 9/9] add test for tintsets --- tests/stdlib/tintsets.nim | 65 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 tests/stdlib/tintsets.nim diff --git a/tests/stdlib/tintsets.nim b/tests/stdlib/tintsets.nim new file mode 100644 index 0000000000000..f859b87aea84c --- /dev/null +++ b/tests/stdlib/tintsets.nim @@ -0,0 +1,65 @@ +import intsets +import std/sets + +from sequtils import toSeq +from algorithm import sorted + +proc sortedPairs[T](t: T): auto = toSeq(t.pairs).sorted +template sortedItems(t: untyped): untyped = sorted(toSeq(t)) + +block: # we use HashSet as groundtruth, it's well tested elsewhere + template testDel(t, t0) = + + template checkEquals() = + doAssert t.len == t0.len + for k in t0: + doAssert k in t + for k in t: + doAssert k in t0 + + doAssert sortedItems(t) == sortedItems(t0) + + template incl2(i) = + t.incl i + t0.incl i + + template excl2(i) = + t.excl i + t0.excl i + + block: + var expected: seq[int] + let n = 100 + let n2 = n*2 + for i in 0..