Fix Issue 17153 – Make std.container.array @nogc #5036

JackStouffer · 2017-01-15T13:34:37Z

Despite Array not using the GC for allocating its elements, it was still unusable for @nogc code because of its use of throw and enforce.

This PR turns those into asserts and makes the majority of the functions @nogc.

JackStouffer · 2017-01-15T13:35:00Z

@andralex requesting your review because this is a breaking change.

aG0aep6G · 2017-01-15T13:51:22Z

std/container/array.d

@@ -142,26 +142,26 @@ private struct RangeT(A)

        E moveFront()
        {
-            version (assert) if (empty || _a >= _outer.length) throw new RangeError();
+            assert(!empty || _a < _outer.length);


Unless I'm having a dumb moment, this should use &&. !(foo || bar) = !foo && !bar. Same mistake throughout.

JackStouffer · 2017-01-15T22:28:17Z

@wilzbach The coverage report that DMD generates and the coverage that appears on codecov are different.

wilzbach

LGTM, but I am not sure how far @andralex is on his journey to make it possible to use exceptions in @nogc?

wilzbach · 2017-02-11T23:57:50Z

@wilzbach The coverage report that DMD generates and the coverage that appears on codecov are different.

That looks pretty weird and I am not sure how to fix this. Maybe @stevepeak (from CodeCov) knows the reason for this issue with CodeCov?

JackStouffer · 2017-02-12T03:45:08Z

but I am not sure how far @andralex is on his journey to make it possible to use exceptions in @nogc?

Requires @safe RC which requires escape analysis which requires DIP1000 to be completed.

PetarKirov

Some late review comments

PetarKirov · 2017-02-12T08:38:29Z

std/container/array.d

-                auto newPayload =
-                    enforce(cast(T*) malloc(sz))[0 .. oldLength];
+
+                auto newPayloadPtr = cast(T*) malloc(sz);


Consider using pureMalloc next time ;)

PetarKirov · 2017-02-12T08:39:36Z

std/container/array.d

            _outer[_b - 1] = value;
        }
        /// Ditto
        T moveBack()
        {
-            enforce(!empty);
+            assert(!empty, "Attempting to mpve the back of an empty Array");


stevepeak · 2017-02-12T15:24:52Z

Hey @wilzbach can you check if the webhook is setup and sending to Codecov? Status was waiting on a webhook. Thank you!

andralex

OK, there's a fair amount of stuff to discuss here.

First, there are a few version(assert) things that throw a different exception. These are safe to commit. Yes, there is a technical breakage, but a palatable one.

Second, there are a few memory allocations that have been changed from enforce to assert. That makes them UB. We don't want that. It should be okay to finish the application upon allocation failure, but not UB.

Third, the bulk is a change in philosophy. Currently, calling front against an empty container caused an exception, so it was defined behavior. This PR makes it undefined. This is definitely not OK for backwards compatibility.

Instead, I propose to make a small PR with the noncontroversial changes in it.

Second, change the remaining instances of enforce with throwing a statically-allocated exception similar to dlang/druntime#1710. That way we'd technically break code (what if someone collects an array of exceptions?) but the scope would be smaller.

The saving grace of this is the documentation doesn't specify current behavior, so we're free to do as we please :). Please accompany this and the following PRs with appropriate documentation changes.

Thanks!

andralex · 2017-02-12T15:15:17Z

std/container/array.d

-                    enforce(cast(T*) malloc(sz))[0 .. oldLength];
+
+                auto newPayloadPtr = cast(T*) malloc(sz);
+                assert(newPayloadPtr, "std.container.Array.reserve failed to allocate memory");


Let's do this: newPayloadPtr || assert(false, "std.container.Array.reserve failed to allocate memory");

andralex · 2017-02-12T15:23:10Z

std/container/array.d

-                    enforce(cast(T*) realloc(_payload.ptr, sz))[0 .. length];
+                auto newPayloadPtr = cast(T*) realloc(_payload.ptr, sz);
+                assert(newPayloadPtr, "std.container.Array.reserve failed to allocate memory");
+                auto newPayload = newPayloadPtr[0 .. length];


Same here, end the app if no memory.

andralex · 2017-02-12T15:23:24Z

std/container/array.d

@@ -547,7 +550,8 @@ Complexity: $(BIGOH 1)
            bool overflow;
            const sz = mulu(elements, T.sizeof, overflow);
            if (overflow) assert(0);
-            auto p = enforce(malloc(sz));
+            auto p = malloc(sz);
+            assert(p);


Same here, end the app if no memory.

andralex · 2017-02-12T15:24:18Z

std/container/array.d

@@ -805,7 +809,7 @@ Complexity: $(BIGOH log(n)).
     */
    void removeBack()
    {
-        enforce(!empty);
+        assert(!empty);
