Skip to content

Add missing functions from Array to Dynarray#13296

Merged
gasche merged 10 commits into
ocaml:trunkfrom
jake-87:trunk
Aug 1, 2024
Merged

Add missing functions from Array to Dynarray#13296
gasche merged 10 commits into
ocaml:trunkfrom
jake-87:trunk

Conversation

@jake-87
Copy link
Copy Markdown
Contributor

@jake-87 jake-87 commented Jul 14, 2024

Added:

  • mem
  • memq
  • find_opt
  • find_index
  • find_map
  • find_mapi

All implementations are based off the implementation in Array. I have tried my best, based on the given comments, and other functions, to add all the correct checks; if I have missed any, please feel free to note or edit, and similar with the quality/number of tests - please inform me if these are not sufficient.

I also apologies; I could not quite figure out the format of the "Changes" file, so I haven't included that change here currently.

Background:
I was attempting to port some code from List to Dynarray; I promptly realized that Dynarray did not have find_opt or similar, and decided that it would probably be easier to implement them myself than raise an issue and wait for someone else to do it.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

This looks fine, but I would propose to systematically (and consistently) call check_same_length outside the loop function:

let rec loop i = ...
in
let res = loop 0 in
check_same_length "..." a ~length;
res

@jake-87
Copy link
Copy Markdown
Contributor Author

jake-87 commented Jul 14, 2024

Sure, that seems reasonable. I'll make that change.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 14, 2024

Could you call check_same_length in mem, memq as well? (They don't call user-provided functions but we can still observe concurrent changes.)

@jake-87
Copy link
Copy Markdown
Contributor Author

jake-87 commented Jul 14, 2024

Sure.

@jake-87
Copy link
Copy Markdown
Contributor Author

jake-87 commented Jul 21, 2024

Hey @gasche; apologies for the bump. Is there anything else you'd like me to implement/change for this patch?

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

I believe that this is correct, and it seems sensible to complete the API with more Array functions.

Per the rules of stdlib maintenance, this needs an approval from another maintainer before we can consider merging this. (Hopefully one of them will receive this in their mailbox and decide to have a look.)

@Octachron
Copy link
Copy Markdown
Member

Looking at the Array module documentation, the new functions are part of the Array scanning section, which also contains exists2 and forall2. However, It might make sense to exclude those n-array functions that require more synchronizations from Dynarray.

Comment thread stdlib/dynarray.mli
@Octachron
Copy link
Copy Markdown
Member

Note that I think it is probably better to left the hypothetical addition of exists2 and for_all2 for another PR which would add the *2 functions to  Dynarray.

@jake-87
Copy link
Copy Markdown
Contributor Author

jake-87 commented Jul 25, 2024

Sorry about that; must have edited the file after I ran the scripts.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 26, 2024

I believe that the new functions are missing @since 5.3 tags in the documentation,

@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 26, 2024

... and a Changes entry in the "Standard library" section.

@jake-87
Copy link
Copy Markdown
Contributor Author

jake-87 commented Jul 27, 2024

Added. Apologies if the Changes file addition is not correct; as mentioned above, I'm not quite sure of the format.

@gasche gasche merged commit 3a39f15 into ocaml:trunk Aug 1, 2024
@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 1, 2024

Merged, thanks!

@jake-87
Copy link
Copy Markdown
Contributor Author

jake-87 commented Aug 2, 2024

Thank you! (and thank you both for putting up with my learnings of the process 😅)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants