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

isEmpty for Deque type #14162

Closed
wants to merge 2 commits into from
Closed

isEmpty for Deque type #14162

wants to merge 2 commits into from

Conversation

disruptek
Copy link
Contributor

We talked about implementing this for collection types; here's one.

@disruptek
Copy link
Contributor Author

Who thinks we should just have a template that tries to len() on the target?

@metagn
Copy link
Collaborator

metagn commented Apr 30, 2020

That, or

template isEmpty(x): bool =
  var result = true
  for _ in x:
    result = false
    break
  result

Works for iterators and efficient for cstring/lists.

@alehander92
Copy link
Contributor

alehander92 commented Apr 30, 2020

the iterator thing should be only a overload for the other cases imo

@metagn
Copy link
Collaborator

metagn commented Apr 30, 2020

If you use len then you're going to have to overload it for every type that has a non-constant len. Everything has an iterator. We should start overloading when we get container concepts like nim-lang/RFCs#50.

@alehander92
Copy link
Contributor

you're right. however an iterator can take a lot of time to produce its first element : in this case you also need to overload isEmpty as it would be probably faster.
overally, one should apply common sense and overload isEmpty if it is really required for his data structure

@alehander92
Copy link
Contributor

I wouldn't want nim to produce C code for a 1 or 2 assignments + a loop just for an access to a len/count field in some cases. hm I don't think it would be optimized well enough but i might be wrong?

@Araq
Copy link
Member

Araq commented Apr 30, 2020

This is how it should be done

template isEmpty*(x: typed): bool = x.len == 0
# added to system.nim

proc isEmpty*(x: cstring): bool {.inline.} = x == nil or x[0] == '\0' # specialization

Yes, it's a system.nim extension but a good one.

@alehander92
Copy link
Contributor

well, an overload for cases where there is no len but there is items might be still cool, but the len, but slower case still would need a manual overload then..

@metagn
Copy link
Collaborator

metagn commented Apr 30, 2020

Maybe len should be renamed to getLen for those (which means len would be deprecated for cstring, but only in C/C++/objc, which is inconvenient). Or some no-op symbol like template lenIsSlow(x: cstring): bool = discard, then you check when compiles(lenIsSlow(x)), which means you can also put it in a concept.

Edit: The types in the lists module don't even have a len overload, you don't need special treatment for them, just compiles(x.len). len for cstring should be deprecated and there should a generic getLen that uses items. Also that cstring overload is bound to be faster than any generic one so it doesn't matter.

@alehander92
Copy link
Contributor

getLen seems confusing , because it's not obvious what the difference is: maybe slowLen or nonConstLen (yeah i know) is still better, but overally encoding complexity in this is hard

@metagn
Copy link
Collaborator

metagn commented Apr 30, 2020

I have a new name, count. Wrote another RFC nim-lang/RFCs#217

PR can do Araq's suggestion for now but maybe with when not declared(x.len) support with my snippet.

@disruptek
Copy link
Contributor Author

Closing for nim-lang/RFCs#217 -- thanks for moving the ball forward!

@disruptek disruptek closed this May 3, 2020
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