From 6e26b552e61e00d3d4a0e4d60f7189e5d780770f Mon Sep 17 00:00:00 2001 From: monarchdodra Date: Fri, 3 Aug 2012 09:40:30 +0300 Subject: [PATCH 1/3] Fixes for issues 8332 and 8333 Consolidating error management --- std/container.d | 112 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 81 insertions(+), 31 deletions(-) diff --git a/std/container.d b/std/container.d index af48167ce1d..754973ae69d 100644 --- a/std/container.d +++ b/std/container.d @@ -212,6 +212,7 @@ module std.container; import core.memory, core.stdc.stdlib, core.stdc.string, std.algorithm, std.conv, std.exception, std.functional, std.range, std.traits, std.typecons, std.typetuple; +import std.string : format; version(unittest) import std.stdio; version(unittest) version = RBDoChecks; @@ -2256,105 +2257,133 @@ Defines the container's primary range, which is a random-access range. private Array _outer; private size_t _a, _b; + private enum string internalMessage = "Array.Range: Internal Error: a > b"; + private enum string invalidatedMessage = "Array.Range: range has become invalid"; + private enum string emptyMessage = ": range is empty"; + private enum string indexMessage = ": index is out of range, i = "; + + private nothrow + bool checkIndex(size_t i) const + { + assert(_a <= _b, internalMessage); + assert(_b <= _outer.length, invalidatedMessage); + return(_a + i < _b); + } + this(Array data, size_t a, size_t b) { + enforce(a <= b); _outer = data; _a = a; _b = b; + assert(_b <= _outer.length, invalidatedMessage); } - @property bool empty() const + @property nothrow + bool empty() const { - assert(_outer.length >= _b); - return _a >= _b; + assert(_a <= _b, internalMessage); + assert(_b <= _outer.length, invalidatedMessage); + return !(_a < _b); } @property Range save() { + assert(_a <= _b, internalMessage); return this; } @property T front() { - enforce(!empty); - return _outer[_a]; + enforce(!empty, text("Array.Range.front", emptyMessage)); + return _outer._data._payload[_a]; } @property T back() { - enforce(!empty); - return _outer[_b - 1]; + enforce(!empty, text("Array.Range.back", emptyMessage)); + return _outer._data._payload[_b - 1]; } @property void front(T value) { - enforce(!empty); - _outer[_a] = move(value); + enforce(!empty, text("Array.Range.front", emptyMessage)); + move(value, _outer._data._payload[_a]); } @property void back(T value) { - enforce(!empty); - _outer[_b - 1] = move(value); + enforce(!empty, text("Array.Range.back", emptyMessage)); + move(value, _outer._data._payload[_b - 1]); } void popFront() { - enforce(!empty); + enforce(!empty, text("Array.Range.popFront", emptyMessage)); ++_a; } void popBack() { - enforce(!empty); + enforce(!empty, text("Array.Range.popBack", emptyMessage)); --_b; } T moveFront() { - enforce(!empty); + enforce(!empty, text("Array.Range.moveFront", emptyMessage)); return move(_outer._data._payload[_a]); } T moveBack() { - enforce(!empty); + enforce(!empty, text("Array.Range.moveBack", emptyMessage)); return move(_outer._data._payload[_b - 1]); } T moveAt(size_t i) { - i += _a; - enforce(i < _b && !empty); + enforce(checkIndex(i), text("Array.Range.moveAt", indexMessage, i)); return move(_outer._data._payload[_a + i]); } T opIndex(size_t i) { - i += _a; - enforce(i < _b && _b <= _outer.length); - return _outer[i]; + enforce(checkIndex(i), text("Array.Range.opIndex", indexMessage, i)); + return _outer._data._payload[_a + i]; } void opIndexAssign(T value, size_t i) { - i += _a; - enforce(i < _b && _b <= _outer.length); - _outer[i] = value; + enforce(checkIndex(i), text("Array.Range.opIndexAssign", indexMessage, i)); + _outer._data._payload[_a + i] = value; } typeof(this) opSlice(size_t a, size_t b) { - return typeof(this)(_outer, a + _a, b + _a); + enforce(a <= b, format("Array.Range.opIndexOpAssign: low index %s is bigger than high index%", + a, b)); + enforce(checkIndex(a), text("Array.Range.opIndexOpAssign", indexMessage, a)); + enforce(checkIndex(b), text("Array.Range.opIndexOpAssign", indexMessage, b)); + return typeof(this)(_outer, a, b); } void opIndexOpAssign(string op)(T value, size_t i) { - enforce(_outer && _a + i < _b && _b <= _outer._payload.length); - mixin("_outer._payload.ptr[_a + i] "~op~"= value;"); + enforce(checkIndex(i), text("Array.Range.opIndexOpAssign", indexMessage, i)); + mixin("_outer._data._payload[_a + i] "~op~"= value;"); } - @property size_t length() const { + void opIndexUnary(string op)(size_t i) + { + enforce(checkIndex(i), text("Array.Range.opIndexUnary", indexMessage, i)); + mixin(op~" _outer._data._payload[_a + i];"); + } + + @property nothrow + size_t length() const + { + assert(_a <= _b, internalMessage); return _b - _a; } } @@ -2365,7 +2394,8 @@ elements. Complexity: $(BIGOH 1) */ - @property bool empty() const + @property nothrow + bool empty() const { return !_data.RefCounted.isInitialized || _data._payload.empty; } @@ -2387,7 +2417,8 @@ Returns the number of elements in the container. Complexity: $(BIGOH 1). */ - @property size_t length() const + @property nothrow + size_t length() const { return _data.RefCounted.isInitialized ? _data._payload.length : 0; } @@ -2398,7 +2429,8 @@ Returns the maximum number of elements the container can store without Complexity: $(BIGOH 1) */ - @property size_t capacity() + @property nothrow + size_t capacity() { return _data.RefCounted.isInitialized ? _data._capacity : 0; } @@ -2516,7 +2548,7 @@ Complexity: $(BIGOH 1) return _data._payload[i]; } - /// ditto +/// ditto void opIndexAssign(T value, size_t i) { enforce(_data.RefCounted.isInitialized); @@ -2529,6 +2561,12 @@ Complexity: $(BIGOH 1) mixin("_data._payload[i] "~op~"= value;"); } +/// ditto + void opIndexUnary(string op)(size_t i) + { + mixin(op~"_data._payload[i];"); + } + /** Returns a new container that's the concatenation of $(D this) and its argument. $(D opBinaryRight) is only defined if $(D Stuff) does not @@ -3047,6 +3085,18 @@ unittest assertThrown(a.replace(r, [42])); assertThrown(a.linearRemove(r)); } +//Test issue 8332 8333: opIndexOpAssign/opIndexUnary +unittest +{ + auto a = Array!int([1, 2, 3]); + a[1] = 0; //Check Array.opIndexAssign + a[1] += 1; //Check Array.opIndexOpAssign + ++a[1]; //Check Array.opIndexUnary + auto r = a[]; + r[1] = 0; //Check Array.Range.opIndexAssign + r[1] += 1; //Check Array.Range.opIndexOpAssign + ++r[1]; //Check Array.Range.opIndexUnary +} // BinaryHeap /** From db4ce80f3b5cdb8c9f55d0bb8a13a464ae445d81 Mon Sep 17 00:00:00 2001 From: monarchdodra Date: Sun, 26 Aug 2012 20:56:21 +0300 Subject: [PATCH 2/3] Fixes for issue 8591 (nothrow) Makes "refCountedPayload" nothrow when ``autoInit == RefCountedAutoInitialize.no`` Also makes Payload.isInitialized nothrow --- std/typecons.d | 49 +++++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/std/typecons.d b/std/typecons.d index 17abb2b2c8b..156dd016fe3 100644 --- a/std/typecons.d +++ b/std/typecons.d @@ -2398,7 +2398,8 @@ if (!is(T == class)) Returns $(D true) if and only if the underlying store has been allocated and initialized. */ - @property bool isInitialized() const + @property nothrow + bool isInitialized() const { return _store !is null; } @@ -2495,14 +2496,6 @@ Assignment operators move(rhs, RefCounted._store._payload); } -/** -Returns a reference to the payload. If (autoInit == -RefCountedAutoInitialize.yes), calls $(D -refCountedEnsureInitialized). Otherwise, just issues $(D -assert(refCountedIsInitialized)). - */ - alias refCountedPayload this; - /** Returns a reference to the payload. If (autoInit == RefCountedAutoInitialize.yes), calls $(D @@ -2511,34 +2504,54 @@ assert(refCountedIsInitialized)). Used with $(D alias refCountedPayload this;), so callers can just use the $(D RefCounted) object as a $(D T). */ - @property ref T refCountedPayload() + static if (autoInit == RefCountedAutoInitialize.yes) { - static if (autoInit == RefCountedAutoInitialize.yes) + @property + ref T refCountedPayload() { RefCounted.ensureInitialized(); + return RefCounted._store._payload; } - else + } + else + { + @property nothrow + ref T refCountedPayload() { assert(RefCounted.isInitialized); + return RefCounted._store._payload; } - return RefCounted._store._payload; } -// - @property ref const(T) refCountedPayload() const +/// ditto + static if (autoInit == RefCountedAutoInitialize.yes) { - static if (autoInit == RefCountedAutoInitialize.yes) + @property + ref const(T) refCountedPayload() const { // @@@ //refCountedEnsureInitialized(); assert(RefCounted.isInitialized); + return RefCounted._store._payload; } - else + } + else + { + @property nothrow + ref const(T) refCountedPayload() const { assert(RefCounted.isInitialized); + return RefCounted._store._payload; } - return RefCounted._store._payload; } + +/** +Returns a reference to the payload. If (autoInit == +RefCountedAutoInitialize.yes), calls $(D +refCountedEnsureInitialized). Otherwise, just issues $(D +assert(refCountedIsInitialized)). + */ + alias refCountedPayload this; } unittest From 725af0367e43d238a2842bf1b77d4f2c8f8b7072 Mon Sep 17 00:00:00 2001 From: monarchdodra Date: Mon, 27 Aug 2012 15:50:08 +0300 Subject: [PATCH 3/3] Arry: new operators,new bounds checking scheme Big Changes for Array: First: I added some new operators: The range wide opUnary and opOpBinary operators, that allow doing an operation on all elements. This was added on Array.Range only (not on Array itself): This is to stay close to native array's behavior: a += 5;//error with both native arrays and Array a[] += 5; //OK with both native arrays and Array Second: Added an invariant: Extracting a range from an Array forces that array's initialization. This is actually VERY valuable, and aids us in a twofold manner: #1: Performance: None of the range's operations need to validate that _data is initialized. #2: Provided the *assert* "_b <= _outer.length", we are guaranteed that "_outer._data._payload[_a .. _b]" is valid, making for quick and easy slicing. Second: A *big* change in philosophy regarding bounds checking: First, both Array and Array.Range implement a private "get" method that always returns a valid raw array (see implementation for details). This has two advantages #1: Centralized logic: Once we have a valid slice, every "op" implementation become trivially simple, and become nothing more that a forward to get's range: #2: Since we are operating on valid slices, we can rely on the language's array bounds checking for bounds enforcement: This actually makes the error messages more verbose, while costing less in developer code, executable size and run-time. --- std/container.d | 274 ++++++++++++++++++++++++++++++------------------ 1 file changed, 170 insertions(+), 104 deletions(-) diff --git a/std/container.d b/std/container.d index 754973ae69d..be89bdf7e76 100644 --- a/std/container.d +++ b/std/container.d @@ -212,7 +212,6 @@ module std.container; import core.memory, core.stdc.stdlib, core.stdc.string, std.algorithm, std.conv, std.exception, std.functional, std.range, std.traits, std.typecons, std.typetuple; -import std.string : format; version(unittest) import std.stdio; version(unittest) version = RBDoChecks; @@ -2064,6 +2063,9 @@ Array type with deterministic control of memory. The memory allocated for the array is reclaimed as soon as possible; there is no reliance on the garbage collector. $(D Array) uses $(D malloc) and $(D free) for managing its own memory. + +Array.Range can provide better performance than Array itself to +access or modify elements. */ struct Array(T) if (!is(T : const(bool))) { @@ -2217,6 +2219,7 @@ struct Array(T) if (!is(T : const(bool))) } private alias RefCounted!(Payload, RefCountedAutoInitialize.no) Data; private Data _data; + private static T[] _emptyDataPayload; //Dummy range to use when _data is not initialized. this(U)(U[] values...) if (isImplicitlyConvertible!(U, T)) { @@ -2257,135 +2260,163 @@ Defines the container's primary range, which is a random-access range. private Array _outer; private size_t _a, _b; - private enum string internalMessage = "Array.Range: Internal Error: a > b"; - private enum string invalidatedMessage = "Array.Range: range has become invalid"; - private enum string emptyMessage = ": range is empty"; - private enum string indexMessage = ": index is out of range, i = "; + //Validates the range, and returns a clean slice to operate on + private @property nothrow + T[] get() + { + assert(_b <= _outer.length, "Array.Range: range has become invalid"); + return _outer._data._payload[_a .. _b]; + } - private nothrow - bool checkIndex(size_t i) const + //Validates the range, and returns a clean slice to operate on + private @property nothrow + const(T)[] get() const { - assert(_a <= _b, internalMessage); - assert(_b <= _outer.length, invalidatedMessage); - return(_a + i < _b); + assert(_b <= _outer.length, "Array.Range: range has become invalid"); + return _outer._data._payload[_a .. _b]; } this(Array data, size_t a, size_t b) { - enforce(a <= b); _outer = data; _a = a; _b = b; - assert(_b <= _outer.length, invalidatedMessage); + get; //Asserts range validity, and validates a <= b via slicing } - + @property nothrow bool empty() const { - assert(_a <= _b, internalMessage); - assert(_b <= _outer.length, invalidatedMessage); - return !(_a < _b); + return get.empty; + } + + @property nothrow + size_t length() const + { + return get.length; } @property Range save() { - assert(_a <= _b, internalMessage); - return this; + //All validations done by constructor + return Range(_outer, _a, _b); + } + + typeof(this) opSlice(size_t a, size_t b) + { + //All validations done by constructor + return typeof(this)(_outer, _a + a, _a + b); } @property T front() { - enforce(!empty, text("Array.Range.front", emptyMessage)); - return _outer._data._payload[_a]; + return get.front; } @property T back() { - enforce(!empty, text("Array.Range.back", emptyMessage)); - return _outer._data._payload[_b - 1]; + return get.back; } @property void front(T value) { - enforce(!empty, text("Array.Range.front", emptyMessage)); - move(value, _outer._data._payload[_a]); + get.front = (value); } @property void back(T value) { - enforce(!empty, text("Array.Range.back", emptyMessage)); - move(value, _outer._data._payload[_b - 1]); + get.back = (value); } - void popFront() + void popFront() nothrow { - enforce(!empty, text("Array.Range.popFront", emptyMessage)); + auto dummy = get; + dummy.popFront(); //Dummy call, but asserts the range is valid, and checks not empty ++_a; } - void popBack() + void popBack() nothrow { - enforce(!empty, text("Array.Range.popBack", emptyMessage)); + auto dummy = get; + dummy.popBack(); //Dummy call, but asserts the range is valid, and checks not empty --_b; } T moveFront() { - enforce(!empty, text("Array.Range.moveFront", emptyMessage)); - return move(_outer._data._payload[_a]); + return move(get.front); } T moveBack() { - enforce(!empty, text("Array.Range.moveBack", emptyMessage)); - return move(_outer._data._payload[_b - 1]); + return move(get.back); } T moveAt(size_t i) { - enforce(checkIndex(i), text("Array.Range.moveAt", indexMessage, i)); - return move(_outer._data._payload[_a + i]); + return move(get[i]); } - T opIndex(size_t i) + void opUnary(string op)() + if(op == "++" || op == "--") { - enforce(checkIndex(i), text("Array.Range.opIndex", indexMessage, i)); - return _outer._data._payload[_a + i]; + mixin(op~"get[];"); } - void opIndexAssign(T value, size_t i) + //Only works for ++ and -- + //For +, this doesn't work with raw arrays anyways + //for -, ~, there is no way to implement it. + void opUnary(string op)() + if(op != "++" && op != "--") { - enforce(checkIndex(i), text("Array.Range.opIndexAssign", indexMessage, i)); - _outer._data._payload[_a + i] = value; + mixin(op~"get[];"); } - typeof(this) opSlice(size_t a, size_t b) + void opOpAssign(string op)(T value) + if(op != "~") { - enforce(a <= b, format("Array.Range.opIndexOpAssign: low index %s is bigger than high index%", - a, b)); - enforce(checkIndex(a), text("Array.Range.opIndexOpAssign", indexMessage, a)); - enforce(checkIndex(b), text("Array.Range.opIndexOpAssign", indexMessage, b)); - return typeof(this)(_outer, a, b); + mixin("get[] "~op~"= value;"); } - void opIndexOpAssign(string op)(T value, size_t i) + T opIndex(size_t i) { - enforce(checkIndex(i), text("Array.Range.opIndexOpAssign", indexMessage, i)); - mixin("_outer._data._payload[_a + i] "~op~"= value;"); + return get[i]; + } + + void opIndexAssign(T value, size_t i) + { + get[i] = value; } void opIndexUnary(string op)(size_t i) + if(op == "++" || op == "--") { - enforce(checkIndex(i), text("Array.Range.opIndexUnary", indexMessage, i)); - mixin(op~" _outer._data._payload[_a + i];"); + mixin(op~" get[i];"); } - @property nothrow - size_t length() const + T opIndexUnary(string op)(size_t i) + if(op != "++" && op != "--") { - assert(_a <= _b, internalMessage); - return _b - _a; + mixin("return "~op~" get[i];"); } + + T opIndexOpAssign(string op)(T value, size_t i) + { + mixin("return get[i] "~op~"= value;"); + } + } + + //Returns a valid slice, either to the payload itself, or to static empty range + private @property nothrow + T[] get() + { + return _data.RefCounted.isInitialized ? _data._payload : _emptyDataPayload; + } + //Returns a valid slice, either to the payload itself, or to static empty range + private @property nothrow + const(T)[] get() const + { + return _data.RefCounted.isInitialized ? _data._payload : _emptyDataPayload; } /** @@ -2397,19 +2428,7 @@ Complexity: $(BIGOH 1) @property nothrow bool empty() const { - return !_data.RefCounted.isInitialized || _data._payload.empty; - } - -/** -Duplicates the container. The elements themselves are not transitively -duplicated. - -Complexity: $(BIGOH n). - */ - @property Array dup() - { - if (!_data.RefCounted.isInitialized) return this; - return Array(_data._payload); + return get.empty; } /** @@ -2430,11 +2449,24 @@ Returns the maximum number of elements the container can store without Complexity: $(BIGOH 1) */ @property nothrow - size_t capacity() + size_t capacity() const { + //The "get" trick doesn't work here: we have to really access the payload return _data.RefCounted.isInitialized ? _data._capacity : 0; } +/** +Duplicates the container. The elements themselves are not transitively +duplicated. + +Complexity: $(BIGOH n). + */ + @property Array dup() + { + if (!_data.RefCounted.isInitialized) return this; + return Array(_data._payload); + } + /** Ensures sufficient capacity to accommodate $(D e) elements. @@ -2470,10 +2502,8 @@ Complexity: $(BIGOH 1) */ Range opSlice() { - // Workaround for bug 4356 - Array copy; - copy._data = this._data; - return Range(copy, 0, length); + _data.RefCounted.ensureInitialized(); + return Range(this, 0, length); } /** @@ -2486,11 +2516,9 @@ Complexity: $(BIGOH 1) */ Range opSlice(size_t a, size_t b) { - enforce(a <= b && b <= length); - // Workaround for bug 4356 - Array copy; - copy._data = this._data; - return Range(copy, a, b); + _data.RefCounted.ensureInitialized(); + //Range.this will be the one enforecing a <= b + return Range(this, a, b); } /** @@ -2510,29 +2538,25 @@ Complexity: $(BIGOH 1) */ @property T front() { - enforce(!empty); - return *_data._payload.ptr; + return get.front; } /// ditto @property void front(T value) { - enforce(!empty); - *_data._payload.ptr = value; + get.front = value; } /// ditto @property T back() { - enforce(!empty); - return _data._payload[$ - 1]; + return get.back; } /// ditto @property void back(T value) { - enforce(!empty); - _data._payload[$ - 1] = value; + get.back = value; } /** @@ -2544,27 +2568,33 @@ Complexity: $(BIGOH 1) */ T opIndex(size_t i) { - enforce(_data.RefCounted.isInitialized); - return _data._payload[i]; + return get[i]; } /// ditto void opIndexAssign(T value, size_t i) { - enforce(_data.RefCounted.isInitialized); - _data._payload[i] = value; + get[i] = value; } /// ditto void opIndexOpAssign(string op)(T value, size_t i) { - mixin("_data._payload[i] "~op~"= value;"); + mixin("get[i] "~op~"= value;"); } /// ditto void opIndexUnary(string op)(size_t i) + if(op == "++" || op == "--") + { + mixin(op~" get[i];"); + } + +/// ditto + T opIndexUnary(string op)(size_t i) + if(op != "++" && op != "--") { - mixin(op~"_data._payload[i];"); + mixin("return "~op~" get[i];"); } /** @@ -2575,7 +2605,8 @@ define $(D opBinary). Complexity: $(BIGOH n + m), where m is the number of elements in $(D stuff) */ - Array opBinary(string op, Stuff)(Stuff stuff) if (op == "~") + Array opBinary(string op, Stuff)(Stuff stuff) + if (op == "~") { // TODO: optimize Array result; @@ -2590,7 +2621,8 @@ stuff) /** Forwards to $(D insertBack(stuff)). */ - void opOpAssign(string op, Stuff)(Stuff stuff) if (op == "~") + void opOpAssign(string op, Stuff)(Stuff stuff) + if (op == "~") { static if (is(typeof(stuff[]))) { @@ -2612,7 +2644,16 @@ Complexity: $(BIGOH n) */ void clear() { - .destroy(_data); + //destroying _data would be problematic for any existing range + //that indexe 0..0, as they *should* remain valid. + //However, destorying _data would break our forced initializaton invariant. + //We prefer setting the length to 0 instead + + //.destroy(_data); + if(_data.RefCounted.isInitialized) + { + _data.length = 0; + } } /** @@ -2790,7 +2831,7 @@ Complexity: $(BIGOH n + m), where $(D m) is the length of $(D stuff) } } - /// ditto +/// ditto size_t insertAfter(Stuff)(Range r, Stuff stuff) { enforce(r._outer._data is _data); @@ -3085,17 +3126,42 @@ unittest assertThrown(a.replace(r, [42])); assertThrown(a.linearRemove(r)); } -//Test issue 8332 8333: opIndexOpAssign/opIndexUnary unittest { - auto a = Array!int([1, 2, 3]); - a[1] = 0; //Check Array.opIndexAssign + auto a = Array!int([1, 1]); + a[1] = 0; //Check Array.opIndexAssign + assert(a[1] == 0); a[1] += 1; //Check Array.opIndexOpAssign - ++a[1]; //Check Array.opIndexUnary + assert(a[1] == 1); + + //Check Array.opIndexUnary + ++a[0]; + assert(a[0] == 2); + assert(+a[0] == +2); + assert(-a[0] == -2); + assert(~a[0] == ~2); + auto r = a[]; - r[1] = 0; //Check Array.Range.opIndexAssign + r[1] = 0; //Check Array.Range.opIndexAssign + assert(r[1] == 0); r[1] += 1; //Check Array.Range.opIndexOpAssign - ++r[1]; //Check Array.Range.opIndexUnary + assert(r[1] == 1); + + //Check Array.Range.opIndexUnary + ++r[0]; + assert(r[0] == 3); + assert(+r[0] == +3); + assert(-r[0] == -3); + assert(~r[0] == ~3); +} +unittest +{ + //Test "range-wide" operations + auto r = Array!int([0, 1, 2])[]; + r += 5; + assert(r.equal([5, 6, 7])); + ++r; + assert(r.equal([6, 7, 8])); } // BinaryHeap