Skip to content

Commit

Permalink
consider calls as complex openarray assignment to iterator params (#2…
Browse files Browse the repository at this point in the history
…4333)

fixes #13417, fixes #19703

When passing an expression to an `openarray` iterator parameter: If the
expression is a statement list (considered "complex"), it's assigned in
a non-deep-copying way to a temporary variable first, then this variable
is used as a parameter. If it's not a statement list, i.e. a call or a
symbol, the parameter is substituted directly with the given expression.
In the case of calls, this results in the call potentially being
executed more than once, or can cause redefined variables in the
codegen.

To fix this, calls are also considered as "complex" assignments to
openarrays, as long as the return type of the call is not `openarray` as
the generated assignment in that case has issues/is unimplemented
(caused a segfault [here in
datamancer](https://github.com/SciNim/Datamancer/blob/47ba4d81bf240a7755b73bc48c1cec9b638d18ae/src/datamancer/dataframe.nim#L1580)).

As for why creating a temporary isn't the default only with exceptions
for things like `nkSym`, the "non-deep-copying" way of assignment
apparently still causes arrays to be copied according to a comment in
the code. I'm not sure to what extent this is true: if it still happens
on ARC/ORC, if it happens for every array length, or if we can fix it by
passing arrays by reference. Otherwise, a more general way to assign to
openarrays might be needed, but I'm not sure if the compiler can easily
do this.
  • Loading branch information
metagn authored Oct 25, 2024
1 parent 9d08f6c commit d303c28
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 1 deletion.
8 changes: 7 additions & 1 deletion compiler/transf.nim
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,12 @@ proc putArgInto(arg: PNode, formal: PType): TPutArgInto =
case arg.kind
of nkStmtListExpr:
return paComplexOpenarray
of nkCall:
if skipTypes(arg.typ, abstractInst).kind in {tyOpenArray, tyVarargs}:
# XXX incorrect, causes #13417 when `arg` has side effects.
return paDirectMapping
else:
return paComplexOpenarray
of nkBracket:
return paFastAsgnTakeTypeFromArg
else:
Expand Down Expand Up @@ -803,7 +809,7 @@ proc transformFor(c: PTransf, n: PNode): PNode =
stmtList.add(newAsgnStmt(c, nkFastAsgn, temp, addrExp, true))
newC.mapping[formal.itemId] = newDeref(temp)
of paComplexOpenarray:
# arrays will deep copy here (pretty bad).
# XXX arrays will deep copy here (pretty bad).
var temp = newTemp(c, arg.typ, formal.info)
addVar(v, temp)
stmtList.add(newAsgnStmt(c, nkFastAsgn, temp, arg, true))
Expand Down
32 changes: 32 additions & 0 deletions tests/iter/titeropenarray.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
block: # issue #13417
var s: seq[int] = @[]
proc p1(): seq[int] =
s.add(3)
@[1,2]

iterator ip1(v: openArray[int]): auto =
for x in v:
yield x

for x in ip1(p1()):
s.add(x)

doAssert s == @[3, 1, 2]

import std / sequtils

block: # issue #19703
iterator combinations[T](s: seq[T], r: Positive): seq[T] =
yield @[s[0], s[1]]

iterator pairwise[T](s: openArray[T]): seq[T] =
yield @[s[0], s[0]]

proc checkSpecialSubset5(s: seq[int]): bool =
toSeq(
toSeq(
s.combinations(2)
).map(proc(a: auto): int = a[0]).pairwise()
).any(proc(a: auto): bool = a == @[s[0], s[0]])

doAssert checkSpecialSubset5 @[1, 2]

0 comments on commit d303c28

Please sign in to comment.