Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DynamicArray.subArray creates array with length bound+1 #280

Merged
merged 1 commit into from Dec 20, 2023

Conversation

DarinM223
Copy link
Contributor

Description

Running this in the REPL:

val arr = DynamicArray.array (10, 0)
val () = DynamicArray.update (arr, 0, 1)
val () = DynamicArray.update (arr, 1, 1)
val () = DynamicArray.update (arr, 2, 1)
val subarr = DynamicArray.toList (DynamicArray.subArray (arr, 0, 2))

raises a Subscript exception, even though the values in the range [0, 2] are set.
Similarly, running this in the REPL:

val arr = DynamicArray.array (10, 0)
val () = DynamicArray.update (arr, 0, 1)
val () = DynamicArray.update (arr, 1, 1)
val () = DynamicArray.update (arr, 2, 1)
val () = DynamicArray.truncate (arr, 2)

will cause arr to be in an invalid state, so running DynamicArray.toList arr or DynamicArray.toVector arr will raise a Subscript exception.

This is because subArray creates an array with the length of the new array bound, even though the comment in the file says that the array should have at least bound+1 elements.

I also modified truncate to set the bound reference even when the array size <= 3 * sz. With this change, truncate can be used to consistently lower the bound of the dynamic array.

Related Issue

Motivation and Context

I want to use DynamicArray.truncate to lower the bound, and I want to use DynamicArray.bound + 1 as the length of all the set elements in the array. Right now truncate often results in an invalid state, and it only lowers the bound in certain cases, which means that the elements from [0, bound] may include zeroed out values.

How Has This Been Tested?

I loaded the updated smlnj-lib.cm file into the REPL and ran this:

> val arr = DynamicArray.array (10, 0);
val arr = - : int DynamicArray.array
> DynamicArray.update (arr, 0, 1);
val it = (): unit
> DynamicArray.update (arr, 1, 1);
val it = (): unit
> DynamicArray.update (arr, 2, 1);
val it = (): unit
> DynamicArray.toList (DynamicArray.subArray (arr, 0, 2));
val it = [1, 1, 1]: int list

> val arr = DynamicArray.array (10, 0);
val arr = - : int DynamicArray.array
> DynamicArray.update (arr, 0, 1);
val it = (): unit
> DynamicArray.update (arr, 1, 1);
val it = (): unit
> DynamicArray.update (arr, 2, 1);
val it = (): unit
> DynamicArray.truncate (arr, 2);
val it = (): unit
> DynamicArray.truncate (arr, 1);
val it = (): unit
> DynamicArray.truncate (arr, 0);
val it = (): unit

- val arr = DynamicArray.array (3, 0);
val arr = - : int DynamicArray.array
- DynamicArray.update (arr, 0, 1);
val it = () : unit
- DynamicArray.update (arr, 1, 1);
val it = () : unit
- DynamicArray.update (arr, 2, 1);
val it = () : unit
- DynamicArray.bound arr;
val it = 2 : int
- DynamicArray.truncate (arr, 2);
val it = () : unit
- DynamicArray.bound arr;
val it = 1 : int

Copy link
Contributor

@JohnReppy JohnReppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me.

@JohnReppy JohnReppy merged commit f02b128 into smlnj:main Dec 20, 2023
@JohnReppy JohnReppy changed the title DynamicArray.subArray creates array with length bound+1 DynamicArray.subArray creates array with length bound+1 Dec 20, 2023
@JohnReppy JohnReppy changed the title DynamicArray.subArray creates array with length bound+1 DynamicArray.subArray creates array with length bound+1 Dec 20, 2023
JohnReppy added a commit that referenced this pull request Dec 20, 2023
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.

None yet

2 participants