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

Array.of_seq applies a circular permutation of one cell to the right on the sequence #7820

Closed
vicuna opened this issue Jul 11, 2018 · 4 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Jul 11, 2018

Original bug ID: 7820
Reporter: thierry.martinez
Status: resolved (set by @xavierleroy on 2018-07-11T17:17:49Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.07.0
Fixed in version: 4.07.1+dev/rc1
Category: standard library

Bug description

With OCaml 4.07.0 and trunk, we have

Array.of_seq (Array.to_seq [| 1; 2; 3 |]);;

  • : int array = [|3; 1; 2|]

In stdlib/array.ml, line 337 (last line of of_rev_list), we have
fill (len-1) tl
whereas it should be
fill (len-2) tl
since hd, which should be assigned to the cell (len - 1), is skipped.

Steps to reproduce

Run the top-level and execute the following line

Array.of_seq (Array.to_seq [| 1; 2; 3 |]);;

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

Comment author: @gasche

Now might be time to repeat my proposition to ask for unit tests for new stdlib functions. In my Batteries experience, this helps a lot for development (quickcheck-style tests help even more).

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

Comment author: thierry.martinez

Even if it is sadly too late, I just proposed a regression test in #1897

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

Comment author: @alainfrisch

Adding more test was actually discussed on #1002 , but left as future work. Perhaps I should have been stricter and waited for those tests before merging.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

Comment author: @xavierleroy

Thanks for the fix. Merged in trunk and in 4.07 branch.

@vicuna vicuna closed this Jul 11, 2018

@vicuna vicuna added stdlib bug labels Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.