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

add Stack.drop #10789

Merged
merged 1 commit into from Jul 5, 2022
Merged

add Stack.drop #10789

merged 1 commit into from Jul 5, 2022

Conversation

zapashcanon
Copy link
Contributor

@zapashcanon zapashcanon commented Nov 23, 2021

Hi,

This PR adds a val drop : 'a t -> unit function to the Stack module.

It behaves as Stack.pop except that it returns () instead of the topmost element.

This is useful when you're e.g. writing a WebAssembly interpreter. :)

More seriously, Sherlocode shows 118 results when searching for ignore (Stack.pop and there are probably more - IIRC Sherlocode doesn't index all opam packages so I bet the real number is higher.

With the feature freeze, I wasn't sure about when this can be merged. I used @since 5.00, let me know if this should be changed.

Copy link
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.

It's a useful proposal, I'm Approving, but indeed that looks like post-freeze material.

(I'm not sure whether the ice age will thaw off before or after the 5.00 release, so I just added a post-freeze milestone.)

@zapashcanon
Copy link
Contributor Author

ping @gasche, I rebased on trunk and I believe it can be merged :)

@gasche
Copy link
Member

gasche commented Jul 4, 2022

We require stdlib changes to be approved by two maintainers instead of one.(cc @Octachron, @nojb)

@zapashcanon
Copy link
Contributor Author

I changed @since 5.0 to @since 5.1.0.

Copy link
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM

I am wondering though if drop : 'a t -> int -> unit may be more generally useful ?

Changes Show resolved Hide resolved
@zapashcanon
Copy link
Contributor Author

@nojb, fixed !

I am wondering though if drop : 'a t -> int -> unit may be more generally useful ?

From the result I get on sherlocode, people seem to drop a single element most of the time.

@xavierleroy
Copy link
Contributor

From the result I get on sherlocode, people seem to drop a single element most of the time.

Agreed. The current API gives no facilities for pushing N elements at a time or popping N elements at a time, so dropping N elements at a time doesn't make much sense to me.

@nojb
Copy link
Contributor

nojb commented Jul 5, 2022

@nojb, fixed !

I am wondering though if drop : 'a t -> int -> unit may be more generally useful ?

From the result I get on sherlocode, people seem to drop a single element most of the time.

Makes sense, the PR diff looks good and it has two approvals. Merging.

@nojb nojb merged commit ed0e517 into ocaml:trunk Jul 5, 2022
@zapashcanon zapashcanon mentioned this pull request Jan 6, 2024
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

4 participants