Skip to content

add Queue.drop#12884

Merged
nojb merged 1 commit into
ocaml:trunkfrom
redianthus:queuedrop
Jun 19, 2024
Merged

add Queue.drop#12884
nojb merged 1 commit into
ocaml:trunkfrom
redianthus:queuedrop

Conversation

@redianthus
Copy link
Copy Markdown
Member

Hi,

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

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

Personally, I am not very enthusiastic about this PR. drop q is just a shorthand for ignore (take q), and not more efficient either. And if we add this operation then we must also add drop_opt and then it starts to feel like too much for such a small addition.

Anyway, let's wait to hear from other developers.

@redianthus
Copy link
Copy Markdown
Member Author

redianthus commented Jan 6, 2024

And if we add this operation then we must also add drop_opt

Why ? And what would be that function ? I don't even see what would be its signature, 'a t -> unit option ? Then you still have to call ignore and it becomes useless. It could be try ignore (take q) with Empty -> () but then naming it _opt is very confusing.

FYI this is really similar to the Stack.drop addition in #10789.

@redianthus redianthus force-pushed the queuedrop branch 2 times, most recently from 2e85f25 to 80661b7 Compare January 11, 2024 11:29
@nojb
Copy link
Copy Markdown
Contributor

nojb commented Jan 11, 2024

And if we add this operation then we must also add drop_opt

Why ?

Well, the general direction is towards exception-less APIs; but indeed there is no technical reason to do so.

And what would be that function ? I don't even see what would be its signature, 'a t -> unit option ?

Whatever it returns (unit option or bool or ...), the point is that it forces the user to handle the case when the stack was empty, if only to explicitly ignore it... Incidentally, in In_channel we have a couple of functions that return unit option:

val really_input : t -> bytes -> int -> int -> unit option

FYI this is really similar to the Stack.drop addition in #10789.

Good point...

Coming back to this addition, I am not strongly against; let's hear from other developers (anyway, more than 1 approval is needed for changes to the standard library).

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

Coming back to this, I am approving it; there's no reason not to have this one if we accept Stack.drop. We still need a second approval before merging, though. @gasche do you have an opinion?

LGTM

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.

Having it for consistency with Stack also sounds reasonable to me. Thanks!

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Jun 18, 2024

@zapashcanon can you rebase the PR? Then we can merge it.

@redianthus
Copy link
Copy Markdown
Member Author

Rebased. :)

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Jun 18, 2024

Rebased. :)

Thanks! The test is failing though:

2024-06-18T13:01:06.2250380Z > ### begin stdout ###
2024-06-18T13:01:06.2331250Z > File "test.ml", line 142, characters 42-48:
2024-06-18T13:01:06.2338920Z > 142 |   Q.add 1 q; Q.drop q; assert (does_raise Q.drop q);
2024-06-18T13:01:06.2343800Z >                                                 ^^^^^^
2024-06-18T13:01:06.2344270Z > Error: This expression has type "'a Q.t -> unit"
2024-06-18T13:01:06.2344840Z >        but an expression was expected of type "'a Q.t -> int"
2024-06-18T13:01:06.2345330Z >        Type "unit" is not compatible with type "int"
2024-06-18T13:01:06.2354790Z > ### end stdout ###
2024-06-18T13:01:06.2359030Z > Action 2/8 (ocamlopt.byte) => failed (Compiling program /Users/runner/work/ocaml/ocaml/testsuite/tests/lib-queue/_ocamltest/tests/lib-queue/test/ocamlopt.byte/test.opt from modules  test.ml: command
2024-06-18T13:01:06.2365320Z > /Users/runner/work/ocaml/ocaml/runtime/ocamlrun /Users/runner/work/ocaml/ocaml/ocamlopt  -I /Users/runner/work/ocaml/ocaml/runtime  -nostdlib -I /Users/runner/work/ocaml/ocaml/stdlib       -o /Users/runner/work/ocaml/ocaml/testsuite/tests/lib-queue/_ocamltest/tests/lib-queue/test/ocamlopt.byte/test.opt   test.ml 
2024-06-18T13:01:06.2368420Z > failed with exit code 2)
2024-06-18T13:01:06.2371500Z > Running test bytecode with 9 actions
2024-06-18T13:01:06.2373220Z > 
2024-06-18T13:01:06.2374530Z > Running action 1/9 (setup-ocamlc.byte-build-env)
2024-06-18T13:01:06.2375570Z > Action 1/9 (setup-ocamlc.byte-build-env) => passed
2024-06-18T13:01:06.2376430Z > 
2024-06-18T13:01:06.2377260Z > Running action 2/9 (ocamlc.byte)
2024-06-18T13:01:06.2378700Z > Compiling program /Users/runner/work/ocaml/ocaml/testsuite/tests/lib-queue/_ocamltest/tests/lib-queue/test/ocamlc.byte/test.byte from modules  test.ml
2024-06-18T13:01:06.2381600Z > Commandline: /Users/runner/work/ocaml/ocaml/runtime/ocamlrun /Users/runner/work/ocaml/ocaml/ocamlc -use-runtime /Users/runner/work/ocaml/ocaml/runtime/ocamlrun -I /Users/runner/work/ocaml/ocaml/runtime -nostdlib -I /Users/runner/work/ocaml/ocaml/stdlib -o /Users/runner/work/ocaml/ocaml/testsuite/tests/lib-queue/_ocamltest/tests/lib-queue/test/ocamlc.byte/test.byte test.ml
2024-06-18T13:01:06.2384340Z >   Redirecting stdout to /Users/runner/work/ocaml/ocaml/testsuite/tests/lib-queue/_ocamltest/tests/lib-queue/test/ocamlc.byte/ocamlc.byte.output 
2024-06-18T13:01:06.2386090Z >   Redirecting stderr to /Users/runner/work/ocaml/ocaml/testsuite/tests/lib-queue/_ocamltest/tests/lib-queue/test/ocamlc.byte/ocamlc.byte.output 
2024-06-18T13:01:06.2387360Z > ### begin stdout ###

@redianthus
Copy link
Copy Markdown
Member Author

Should be fixed now, sorry..

@nojb nojb merged commit a36d997 into ocaml:trunk Jun 19, 2024
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