-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Extend options stdlib #24849
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
base: devel
Are you sure you want to change the base?
Extend options stdlib #24849
Conversation
eca83a3
to
ee24d96
Compare
The failure being reported in CI is:
I may be missing something, but that seems unrelated to the change in this PR |
Correct, don't worry. |
for results, I've been planning to reuse tricks like https://github.com/nim-lang/Nim/blob/version-2-2/lib/pure/collections/sequtils.nim#L97 to avoid most of the extra copies that are unnecessary - this implementation is very heavy on copies in general, often in a way that is tricky for the compiler to get rid of. a second trick to use is to verify that the code efficiently moves data around and/or uses cursors where applicable, ie in |
In particular, see https://github.com/arnetheduck/nim-results/blob/df8113dda4c2d74d460a8fa98252b0b771bf1f27/results.nim#L1430 for a much more efficient way to inject a variable into a template - there is also a variation on the same theme when genericsopensym is not enabled. |
Sorry for the delayed update. Rebased and pushed a new version that should prevent copies. CI failures look unrelated to my changes. Note that I removed the ‘valueOr’ template as I couldn’t figure out how to implement it without requiring a copy. I didn’t want to block the other changes on that one snag, though |
outcome = action | ||
outcome | ||
|
||
template filterIt*[T](value: Option[T], action: untyped): Option[T] = |
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.
Better rename the action
to pred
for consistency with other filter templates in stdlib
block: | ||
var outcome = value | ||
outcome.withValue(it): | ||
if not action: | ||
outcome = none(T) | ||
do: | ||
outcome = none(T) | ||
outcome |
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.
Why not just?
block: | |
var outcome = value | |
outcome.withValue(it): | |
if not action: | |
outcome = none(T) | |
do: | |
outcome = none(T) | |
outcome | |
block: | |
evalOnceAs(local, value) | |
template it(): auto {.inject, used.} = unsafeGet(local) | |
if local.isSome and action: | |
value | |
else: | |
none(T) |
You can't simply use withValue
here as an expression with the current implementation, unfortunately. This might be worth looking into.
outcome | ||
|
||
template applyIt*[T](value: Option[T], action: untyped) = | ||
## Executes a code block if the `Option` is `some`, assigning the value to a variable named `it` |
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.
To follow the common apply
semantics this should work:
var o = "foo"
echo o.applyIt(it = "bar")
Then the docs should clearly draw attention to mapIt
being able to return another type and applyIt
just working on the held value of the same type.
Apply should return the same type being passed to it, but this just does whatever (or doesn't?). To think more about it, Option is not exactly a container, this template looks confusing applied to it and redundant.
This changes adds more procs to the
options
stdlib. These are convenience functions that make life easier when interacting with Options -- I've wound up adding them to just about every app I have written with Nim.