From edb5a1a5c6b8381c41561f303aa3588ff5fbab69 Mon Sep 17 00:00:00 2001 From: Kier Davis Date: Sat, 18 Jun 2016 15:43:32 +0100 Subject: [PATCH 1/4] Fix clear() on CountTable The record tuples used in CountData.data don't contain an 'hcode' member, unlike Table and OrderedTable, causing the existing clearImpl() implementation to break when attempting to assign to t.data[i].hcode. --- lib/pure/collections/tableimpl.nim | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/pure/collections/tableimpl.nim b/lib/pure/collections/tableimpl.nim index 1bbf19ee93..f736320081 100644 --- a/lib/pure/collections/tableimpl.nim +++ b/lib/pure/collections/tableimpl.nim @@ -142,7 +142,8 @@ template delImpl() {.dirty, immediate.} = template clearImpl() {.dirty, immediate.} = for i in 0 .. Date: Sat, 18 Jun 2016 15:45:31 +0100 Subject: [PATCH 2/4] Add tests for tables.clear() This should reduce the chance of regressions. --- tests/collections/ttables.nim | 21 +++++++++++++++++++++ tests/collections/ttablesref.nim | 25 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/tests/collections/ttables.nim b/tests/collections/ttables.nim index a8a182a789..773a8f3b7e 100644 --- a/tests/collections/ttables.nim +++ b/tests/collections/ttables.nim @@ -134,6 +134,27 @@ block mpairsTableTest1: block SyntaxTest: var x = toTable[int, string]({:}) +block clearTableTest: + var t = data.toTable + assert t.len() != 0 + t.clear() + assert t.len() == 0 + +block clearOrderedTableTest: + var t = data.toOrderedTable + assert t.len() != 0 + t.clear() + assert t.len() == 0 + +block clearCountTableTest: + var t = initCountTable[string]() + t.inc("90", 3) + t.inc("12", 2) + t.inc("34", 1) + assert t.len() != 0 + t.clear() + assert t.len() == 0 + proc orderedTableSortTest() = var t = initOrderedTable[string, int](2) for key, val in items(data): t[key] = val diff --git a/tests/collections/ttablesref.nim b/tests/collections/ttablesref.nim index 32494f1f28..12af1ccbb8 100644 --- a/tests/collections/ttablesref.nim +++ b/tests/collections/ttablesref.nim @@ -141,6 +141,31 @@ block anonZipTest: let values = @[1, 2, 3] doAssert "{a: 1, b: 2, c: 3}" == $ toTable zip(keys, values) +block clearTableTest: + var t = newTable[string, float]() + t["test"] = 1.2345 + t["111"] = 1.000043 + t["123"] = 1.23 + assert t.len() != 0 + t.clear() + assert t.len() == 0 + +block clearOrderedTableTest: + var t = newOrderedTable[string, int](2) + for key, val in items(data): t[key] = val + assert t.len() != 0 + t.clear() + assert t.len() == 0 + +block clearCountTableTest: + var t = newCountTable[string]() + t.inc("90", 3) + t.inc("12", 2) + t.inc("34", 1) + assert t.len() != 0 + t.clear() + assert t.len() == 0 + orderedTableSortTest() echo "true" From 449960bf7e3cad1aa4f3e38e39582ac220237ecb Mon Sep 17 00:00:00 2001 From: Kier Davis Date: Sat, 9 Jul 2016 17:34:01 +0100 Subject: [PATCH 3/4] Add a fix for clear() on non-ref types by adding a missing 'var' annotation to the type signature However, this fix won't take effect until a compiler bug (#4448) is fixed. Until then, the codebase functions identically to how it did before this commit (calls to clear() fail to compile for Table/OrderedTable/CountTable as the argument is immutable). --- lib/pure/collections/tables.nim | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pure/collections/tables.nim b/lib/pure/collections/tables.nim index e454a43cbe..3a0c50c508 100644 --- a/lib/pure/collections/tables.nim +++ b/lib/pure/collections/tables.nim @@ -85,7 +85,7 @@ template dataLen(t): expr = len(t.data) include tableimpl -proc clear*[A, B](t: Table[A, B] | TableRef[A, B]) = +proc clear*[A, B](t: var Table[A, B] | TableRef[A, B]) = ## Resets the table so that it is empty. clearImpl() @@ -425,7 +425,7 @@ proc len*[A, B](t: OrderedTable[A, B]): int {.inline.} = ## returns the number of keys in `t`. result = t.counter -proc clear*[A, B](t: OrderedTable[A, B] | OrderedTableRef[A, B]) = +proc clear*[A, B](t: var OrderedTable[A, B] | OrderedTableRef[A, B]) = ## Resets the table so that it is empty. clearImpl() t.first = -1 @@ -754,7 +754,7 @@ proc len*[A](t: CountTable[A]): int = ## returns the number of keys in `t`. result = t.counter -proc clear*[A](t: CountTable[A] | CountTableRef[A]) = +proc clear*[A](t: var CountTable[A] | CountTableRef[A]) = ## Resets the table so that it is empty. clearImpl() t.counter = 0 From 8e843354e12fdaf7697cfdfe9cd4efd83737db18 Mon Sep 17 00:00:00 2001 From: Kier Davis Date: Sat, 9 Jul 2016 18:21:37 +0100 Subject: [PATCH 4/4] Disable failing tests for tables.clear() The tests for tables.clear() in tests/collections/ttables.nim currently fail as a result of #4448, so I've wrapped them in a 'when false' to disable them until the bug is fixed. --- tests/collections/ttables.nim | 38 ++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/tests/collections/ttables.nim b/tests/collections/ttables.nim index 773a8f3b7e..59fef49208 100644 --- a/tests/collections/ttables.nim +++ b/tests/collections/ttables.nim @@ -134,26 +134,28 @@ block mpairsTableTest1: block SyntaxTest: var x = toTable[int, string]({:}) -block clearTableTest: - var t = data.toTable - assert t.len() != 0 - t.clear() - assert t.len() == 0 +# Until #4448 is fixed, these tests will fail +when false: + block clearTableTest: + var t = data.toTable + assert t.len() != 0 + t.clear() + assert t.len() == 0 -block clearOrderedTableTest: - var t = data.toOrderedTable - assert t.len() != 0 - t.clear() - assert t.len() == 0 + block clearOrderedTableTest: + var t = data.toOrderedTable + assert t.len() != 0 + t.clear() + assert t.len() == 0 -block clearCountTableTest: - var t = initCountTable[string]() - t.inc("90", 3) - t.inc("12", 2) - t.inc("34", 1) - assert t.len() != 0 - t.clear() - assert t.len() == 0 + block clearCountTableTest: + var t = initCountTable[string]() + t.inc("90", 3) + t.inc("12", 2) + t.inc("34", 1) + assert t.len() != 0 + t.clear() + assert t.len() == 0 proc orderedTableSortTest() = var t = initOrderedTable[string, int](2)