Fixes for issues 8332 and 8333 #736

monarchdodra · 2012-08-03T06:41:24Z

This commit first fixes issues 8332 and 8333:
http://d.puremagic.com/issues/show_bug.cgi?id=8332
http://d.puremagic.com/issues/show_bug.cgi?id=8333

Basically, Array and Array.Range did not implement opIndexUnary. Also, Array.Range.opIndexOpAssign was not compiling due to a typo type bug.
These issues were opened by me, and are not assigned to anyone. I'm not sure about the protocol regarding bugs... Should (can) I assign them to myself? Will someone else (andralex?) close them?

I also added:
_Documentation of invariants/enforcements of Array.Range for future developers.
*Stricter enforcement in Array.Range: Quite a few illegal operations were executed without complaining.
*Direct call to _outer._data._payload in Array.Range:
*_avoids double checking of "_data.RefCounted.isInitialized", which should already be validated by either of "!empty" or "i < _b && _b <= length".
*_Also avoids double check of "empty etc..."
*_Allows more efficient implementations using move(value, target)
*Unit tests.

andralex · 2012-08-06T03:49:48Z

std/container.d

@@ -2265,37 +2283,38 @@ Defines the container's primary range, which is a random-access range.

        @property bool empty() const
        {
-            assert(_outer.length >= _b);
-            return _a >= _b;
+            enforce(_b <= _outer.length);


This will be slow. I suggest you replace uses of enforce with assert until we come up with a systematic handling of this matter.

In general, we haven't been using exceptions in range functions, and I don't think that we should start now. They need to be fast, and I think that it's perfectly reasonable to require that that the caller do any necessary verification. Not to mention, if empty requires verification from the caller's perspective, I think that there's something wrong with the design.

andralex · 2012-08-13T02:17:29Z

will get to this after Aug 20

DmitryOlshansky · 2012-08-17T09:19:04Z

std/container.d

@@ -2253,11 +2253,29 @@ Defines the container's primary range, which is a random-access range.
     */
    struct Range
    {
+
+/**


Oh my DDoc section.... I'm sure end users do not need this.
Please choose some other way to highlight important implementation notes.

andralex · 2012-08-20T03:44:08Z

review due for next week

monarchdodra · 2012-08-20T20:02:44Z

I read all the comments. Will provide a new version in 2 days. TY.

monarchdodra · 2012-08-24T10:17:32Z

So I consolidated the enforces and asserts a bit. I made them much more verbose. Not sure if this is wanted or not?
If not, I'll remove them.

That said, I did move some enforced checks to assert checks, so the overall result should be an improvement.

I also did some minor nothrow/const corrections here and there...

monarchdodra · 2012-08-24T10:19:22Z

std/typecons.d

@@ -2491,46 +2492,58 @@ Assignment operators
 Returns a reference to the payload. If (autoInit ==
 RefCountedAutoInitialize.yes), calls $(D
 refCountedEnsureInitialized). Otherwise, just issues $(D
-assert(refCountedIsInitialized)).
- */
-    alias refCountedPayload this;


This code was not removed, but just move, because the compiler couldn't "see" reCountedPayload inside the static ifs.

monarchdodra · 2012-08-26T18:21:06Z

Also contains correction 8591:
http://d.puremagic.com/issues/show_bug.cgi?id=8591

repeatedly · 2012-08-26T19:10:00Z

std/container.d

@@ -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 has become invalid";
+        private enum string emptyMessage = ": rangeIsEmpty";


Why is this message camel case? 'range is empty' is better.

Good point! Typo on my end.

Consolidating error management

Makes "refCountedPayload" nothrow when ``autoInit == RefCountedAutoInitialize.no`` Also makes Payload.isInitialized nothrow

andralex · 2012-08-26T19:34:39Z

std/container.d

+        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 = ";


why stutter?

private enum string zis = "this", zat = "that", zeOzer = "the other";

andralex · 2012-08-26T19:42:13Z

Almost there. I think I found a bug in opIndexUnary.

monarchdodra · 2012-08-27T12:51:03Z

I was not satisfied with what I had done. I did some "big" changes, which I think makes everything better in all regards:

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.

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.

monarchdodra · 2012-08-27T21:24:22Z

I did some extra testing, and my new code is hitting a bug:
Array!Int a;
auto r = a[];
f.front; //This does an Array.front assertion, as should be expected and desired
a[].front; //But doing it "one-liner" crashes the program ... ?

Any thoughts? I tried re-doing the 4356 fix in opSlice (the bug is marked as fixed, so that shouldn't be it anyways), but no change.

Help!

monarchdodra · 2012-08-28T18:00:37Z

This pull is becoming too complicated. I am splitting it into different pulls. I'll do a more robust method attribute coverage and error handling in a different pull. Closing.

andralex reviewed Aug 6, 2012
View reviewed changes

DmitryOlshansky reviewed Aug 17, 2012
View reviewed changes

monarchdodra reviewed Aug 24, 2012
View reviewed changes

repeatedly reviewed Aug 26, 2012
View reviewed changes

monarchdodra added 2 commits August 26, 2012 21:17

Fixes for issues 8332 and 8333

6e26b55

Consolidating error management

Fixes for issue 8591 (nothrow)

db4ce80

Makes "refCountedPayload" nothrow when ``autoInit == RefCountedAutoInitialize.no`` Also makes Payload.isInitialized nothrow

andralex reviewed Aug 26, 2012
View reviewed changes

monarchdodra closed this Aug 28, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for issues 8332 and 8333 #736

Fixes for issues 8332 and 8333 #736

monarchdodra commented Aug 3, 2012

andralex Aug 6, 2012

jmdavis Aug 13, 2012

andralex commented Aug 13, 2012

DmitryOlshansky Aug 17, 2012

andralex commented Aug 20, 2012

monarchdodra commented Aug 20, 2012

monarchdodra commented Aug 24, 2012

monarchdodra Aug 24, 2012

monarchdodra commented Aug 26, 2012

repeatedly Aug 26, 2012

monarchdodra Aug 26, 2012

andralex Aug 26, 2012

andralex commented Aug 26, 2012

monarchdodra commented Aug 27, 2012

monarchdodra commented Aug 27, 2012

monarchdodra commented Aug 28, 2012

Fixes for issues 8332 and 8333 #736

Fixes for issues 8332 and 8333 #736

Conversation

monarchdodra commented Aug 3, 2012

andralex Aug 6, 2012

Choose a reason for hiding this comment

jmdavis Aug 13, 2012

Choose a reason for hiding this comment

andralex commented Aug 13, 2012

DmitryOlshansky Aug 17, 2012

Choose a reason for hiding this comment

andralex commented Aug 20, 2012

monarchdodra commented Aug 20, 2012

monarchdodra commented Aug 24, 2012

monarchdodra Aug 24, 2012

Choose a reason for hiding this comment

monarchdodra commented Aug 26, 2012

repeatedly Aug 26, 2012

Choose a reason for hiding this comment

monarchdodra Aug 26, 2012

Choose a reason for hiding this comment

andralex Aug 26, 2012

Choose a reason for hiding this comment

andralex commented Aug 26, 2012

monarchdodra commented Aug 27, 2012

monarchdodra commented Aug 27, 2012

monarchdodra commented Aug 28, 2012