Skip to content

Conversation

monarchdodra
Copy link
Collaborator

I've been wanting to do this for a while, but kept getting side-tracked by the amount of things to do in std.container.Array.

I made this pull "minimal":
It makes std.container.Array (and it's Range) assert on any out of bounds conditions. Furhermore, I made sure the Error thrown is actually a RangeError. Range only makes minial validation about its own boundaries, before delegating to Array (avoids code duplication/double validation (except for "move" functions, which don't exist in Array)).

Last but not least, I corrected a little issue in Range, where doing "0 .. 0" operations on a non-initialized Array threw an error, when there is actually no reason to.

@Poita
Copy link
Contributor

Poita commented Jan 1, 2014

Index checks should be inside version(D_NoBoundsChecks) else { ... }

@monarchdodra
Copy link
Collaborator Author

Index checks should be inside version(D_NoBoundsChecks) else { ... }

D_NoBoundsChecks only means that the code is being compiled with noboundscheck, which has the added effect of removing bounds checking in safe code.

But by default, there is no bounds check in @system code in release mode.

That said, I think the design here is correct: The asserts verify "logical" constraints in non-release mode. Once we do compile in release, the asserts are removed, but there is still some "natural" array bounds checking to prevent undefined behavior and clobbering memory:

auto r = Array!int([0, 1, 2, 3])[1 .. 2];

//This will range error in non-release.
//It will return "3" in release.
auto a = r [3];


//This will range error in non-release.
//This will *also* range error in release.
//In release + noboundscheck, it will have undefined behavior.
auto a = r [7];

@ghost
Copy link

ghost commented Jan 6, 2014

Auto-merge toggled on

@ghost
Copy link

ghost commented Jan 6, 2014

LGTM.

ghost pushed a commit that referenced this pull request Jan 6, 2014
Make std.container.Array assert.
@ghost ghost merged commit 3f2fd10 into dlang:master Jan 6, 2014
@monarchdodra monarchdodra deleted the ArrayAssert3 branch April 1, 2014 06:34
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants