Fixes for issues 8332 and 8333#736
Fixes for issues 8332 and 8333#736monarchdodra wants to merge 3 commits intodlang:masterfrom monarchdodra:arrayBug
Conversation
std/container.d
Outdated
There was a problem hiding this comment.
This will be slow. I suggest you replace uses of enforce with assert until we come up with a systematic handling of this matter.
There was a problem hiding this comment.
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.
|
will get to this after Aug 20 |
std/container.d
Outdated
There was a problem hiding this comment.
Oh my DDoc section.... I'm sure end users do not need this.
Please choose some other way to highlight important implementation notes.
|
review due for next week |
|
I read all the comments. Will provide a new version in 2 days. TY. |
|
So I consolidated the enforces and asserts a bit. I made them much more verbose. Not sure if this is wanted or not? 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... |
There was a problem hiding this comment.
This code was not removed, but just move, because the compiler couldn't "see" reCountedPayload inside the static ifs.
|
Also contains correction 8591: |
std/container.d
Outdated
There was a problem hiding this comment.
Why is this message camel case? 'range is empty' is better.
There was a problem hiding this comment.
Good point! Typo on my end.
Consolidating error management
Makes "refCountedPayload" nothrow when ``autoInit == RefCountedAutoInitialize.no`` Also makes Payload.isInitialized nothrow
std/container.d
Outdated
There was a problem hiding this comment.
why stutter?
private enum string
zis = "this",
zat = "that",
zeOzer = "the other";
|
Almost there. I think I found a bug in opIndexUnary. |
|
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: 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: 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 |
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.
|
I did some extra testing, and my new code is hitting a bug: 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! |
|
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. |
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.