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

fixes #8577, #8580, other bug fixes #8584

Merged
merged 4 commits into from
Aug 14, 2018
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 38 additions & 32 deletions lib/pure/collections/sequtils.nim
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,22 @@ import macros
when not defined(nimhygiene):
{.pragma: dirty.}


macro evalOnce(v, exp: untyped, letAssigneable: static[bool]): untyped =
expectKind(v, nnkIdent)
var val = exp

result = newStmtList()
# If `exp` is not a symbol we evaluate it once here and then use the temporary
# symbol as alias
if exp.kind != nnkSym and letAssigneable:
val = genSym()
result.add(newLetStmt(val, exp))

result.add(
newProc(name = genSym(nskTemplate, $v), params = [getType(untyped)],
body = val, procType = nnkTemplateDef))

proc concat*[T](seqs: varargs[seq[T]]): seq[T] =
## Takes several sequences' items and returns them inside a new sequence.
##
Expand Down Expand Up @@ -635,30 +651,7 @@ template mapIt*(s, typ, op: untyped): untyped =
result.add(op)
result

# This is needed in order not to break the bootstrap, the fallback
# implementation is a "dumb" let that won't work in some cases (eg. when `exp`
# is an openArray)
when declared(macros.symKind):
macro evalOnce(v, exp: untyped): untyped =
expectKind(v, nnkIdent)
var val = exp

result = newStmtList()

# Not a parameter we can pass as-is, evaluate and store in a temporary
# variable
if exp.kind != nnkSym or exp.symKind != nskParam:
val = genSym()
result.add(newLetStmt(val, exp))

result.add(
newProc(name = genSym(nskTemplate, $v), params = [getType(untyped)],
body = val, procType = nnkTemplateDef))
else:
macro evalOnce(v, exp: untyped): untyped =
result = newLetStmt(v, exp)

template mapIt*(s, op: untyped): untyped =
template mapIt*(s: typed, op: untyped): untyped =
## Convenience template around the ``map`` proc to reduce typing.
##
## The template injects the ``it`` variable which you can use directly in an
Expand All @@ -675,16 +668,16 @@ template mapIt*(s, op: untyped): untyped =
block:
var it{.inject.}: type(items(s));
op))
var result: seq[outType]
when compiles(s.len):
evalOnce(t, s)
# using t_D20180808T211500 to avoid https://github.com/nim-lang/Nim/issues/8580
evalOnce(t_D20180808T211500, s, type(s) isnot openArray)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nasty. Using a block: is a somewhat cleaner workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of checking whether s is an openArray I'd just add another compiles((let _ = s)) to the when above.

Copy link
Member

Choose a reason for hiding this comment

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

This usage of compiles seems to be a hack... :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but I can't think of a better way to shunt the bare openArray literals into the other code path

Copy link
Member Author

Choose a reason for hiding this comment

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

This is nasty. Using a block: is a somewhat cleaner workaround.

used a block; much cleaner, thanks!

compiles((let _ = s))

had thought of that but will likely lead to other bugs and hide other issues

Yep, but I can't think of a better way to shunt the bare openArray literals into the other code path

not sure what you're suggesting besides compiles ; what could go wrong with type(s) isnot openArray ?

Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed here https://gitter.im/nim-lang/Nim?at=5b6c0cf3637ee66082c775e8, I changed to compiles((let _ = s)) (see comments in code)

var i = 0
result = newSeq[outType](t.len)
for it {.inject.} in t:
var result = newSeq[outType](t_D20180808T211500.len)
for it {.inject.} in t_D20180808T211500:
result[i] = op
i += 1
else:
result = @[]
var result: seq[outType] = @[]
for it {.inject.} in s:
result.add(op)
result
Expand Down Expand Up @@ -1067,9 +1060,22 @@ when isMainModule:
doAssert mapLiterals(([1], ("abc"), 2), `$`, nested=true) == (["1"], "abc", "2")

block: # mapIt with openArray
when declared(macros.symKind):
proc foo(x: openArray[int]): seq[int] = x.mapIt(it + 1)
doAssert foo([1,2,3]) == @[2,3,4]
proc foo(x: openArray[int]): seq[int] = x.mapIt(it + 1)
doAssert foo([1,2,3]) == @[2,3,4]

block: # mapIt with direct openArray
# see https://github.com/_render_node/MDExOlB1bGxSZXF1ZXN0MjA3MTQ3MTIw/pull_requests/timeline#issuecomment-411614711
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the github.com/_render_node/.. link?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... this URL doesn't load.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed it

doAssert openArray[int]([1,2]).mapIt(it) == @[1,2]
Copy link
Contributor

Choose a reason for hiding this comment

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

A better test would be to create a proc that takes an openarray. Pass it a list and call mapIt on the argument.

Frankly, I'm surprised this works. openArray is a pseudo-type AFAIK.

Copy link
Member Author

Choose a reason for hiding this comment

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

i already have a test for that, see "mapIt with openArray"


block:
var counter = 0
proc getInput():auto =
counter.inc
[1, 2]
doAssert getInput().mapIt(it*2).mapIt(it*10) == @[20, 40]
# make sure argument evaluated only once, analog to
# https://github.com/nim-lang/Nim/issues/7187 test case
doAssert counter == 1

block: # mapIt with invalid RHS for `let` (#8566)
type X = enum
Expand Down