-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
stdlib: add List.is_empty
#10464
stdlib: add List.is_empty
#10464
Conversation
Implementation switched to use structural equality rather than pattern matching (thanks to a hint by @dra27). It seems the compiler has an easier time eliminating the branch this way: |(seq
-| (let (is_empty/80 = (function l/82 (if l/82 0a 1a)))
+| (let (is_empty/80 = (function l/82 (== l/82 0a)))
| (setfield_ptr(root-init) 0 (global Test!) is_empty/80))
| 0a) |
Note: the structural equality version is better now, but only because the pattern-matching compiler generates sub-optimal code. |
I would prefer not adding functions that force sequences. |
With
Oops, we still miss More seriously: the one true way to work with lists in OCaml is pattern-matching. Every library function on lists that distracts from pattern-matching makes me sad. |
I normally write
Is that less efficient? Or in poor taste? Or perhaps just hard to read? |
@johnwhitington: it is less efficient indeed, at least with non-flambda compilers. This is partly due to the allocation cost of the module All_empty = struct
let via_inline l = List.for_all (( = ) []) l
let via_inline_weak = List.for_all (( = ) [])
let via_inline_expanded l = List.for_all (fun x -> x = []) l
let via_stdlib l = List.for_all List.is_empty l
end When benchmarked with
You may be interested in https://blog.janestreet.com/the-dangers-of-being-too-partial/. As mentioned above, my main concern w.r.t. performance is not that experienced users can't hand-roll an efficient implementation, but rather that too many people seem to reach for the very inefficient I do also happen to think there is a readability argument in favour of |
FWIW, from my own teaching, I would say that the existence of |
I don't think it is fair to compare |
Obviously pattern matching handles most of one's daily needs (see the fact that few have complained that this was missing over the years), but I find myself writing similar things often enough that I think it wouldn't be bad to have this in the standard library. And as has been said, |
We already have
And since the one true way to work with recursive data structures are recursive functions, one should discourage the use of iterators like |
I stand by my comment that the one true way to inspect lists, options, and any other value of inductive type is by pattern-matching. If there's clear support for this PR, I'll shut up. Otherwise, I'll move to close. |
I'd like to add my support then. When predicates are needed I find the lack of such functions make the code noisy and less obvious than it could be; see for example @johnwhitington's examples. Also I'd like to be able to write |
Indeed. To reiterate, sometimes, you just want to pass |
I am in favour of adding this. |
To follow up on my earlier comment: now that #10681 is merged, we are in the ideal world, so there's no reason to prefer equality to pattern-matching anymore (at least in native mode). |
@lthls I don't understand the relevance here. If you want to run map over a collection of lists to get a collection of booleans stating which are empty and which are not empty, you still need |
@pmetzger - the comment's about the implementation, rather than the need for the function. @craigfe originally gave: let is_empty = function
| [] -> true
| _ :: _ -> false which #10681 in ocamlopt the code compiled down to (amd64): cmpq $1, %rax
je .L100
movl $1, %eax
ret
.align 4
.L100:
movl $3, %eax
ret vs cmpq $1, %rax
sete %al
movzbq %al, %rax
leaq 1(%rax,%rax), %rax
ret for the code presently in the PR. However, trunk (i.e. 4.14) now generates better code for the original pattern matching version: andq $1, %rax
leaq 1(%rax,%rax), %rax
ret |
Ah, indeed, I was confused about what thread of the conversation was being invoked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge with second approval.
I thought programming against |
As of ocaml#10681, the native compiler is able to simplify this pattern-matching to use bitwise operations rather than an explicit branch (as was previously the case) or an integer comparison (as is the case for the implementation using `( = )`).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally in Xavier's camp that pattern-matching is the nice way to inspect values. This being said, some of the arguments here are convincing, and I agree with the point of @dra27 in #11680 that it's too tempting for beginners to write List.length li = 0
instead. Finally, there are many other containers that provide this function, notably Stack
, Queue
, Seq
, Set
, Map
, so the change arguably adds consistency. Approved.
Adds a fairly self-explanatory function:
This can be found in both
containers
andbase
.The rationale for proposing it is that I see it hand-rolled as
List.length l = 0
relatively frequently in the wild, which is inefficient. It's possible to usel = []
instead, but I think a dedicated function would encourage users to fall into this more efficient pattern organically. (Plus, some codebases intentionally put barriers in the way of polymorphic equality.)Notes on consistency:
Map.is_empty
;{String,Bytes,Array}.is_empty
, with the concensus being that these should be implemented in terms of compiler primitives;Seq.is_empty
too, though it does force an unused element. (Containers and Base provide it.)