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

timotheecour · 2018-08-09T04:30:59Z

/c @Araq @LemonBoy more fixes coming after this PR, eg: #8586 (+ 2 others) which depend on this to be merged first ; so can we merge this sooner rather than later?

fixes mapIt still can't be used with openArray even after #8543, #8567: fails during bootstrap #8577
builds on and improves Yet another change to evalOnce #8578 (from @LemonBoy) to also fix these:
fixes let t=s.mapIt(it) gives Error: redefinition of 't' #8580
fixes Yet another change to evalOnce #8578 (comment)
avoids duplicate initialization of result in mapIt
adds more test cases

LemonBoy

One more failing test case:

[].mapIt(it)

LemonBoy · 2018-08-09T07:44:18Z

lib/pure/collections/sequtils.nim

  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)


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

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

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

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

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 ?

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

timotheecour · 2018-08-09T09:43:03Z

One more failing test case: [].mapIt(it)

illegal (just as let a =@[] is), however added test case for (typed) empty seqs.

PTAL (all comments addressed)

timotheecour · 2018-08-09T10:02:31Z

PTAL all comments addressed (after fix to compiles((let _ = s))

kaushalmodi · 2018-08-10T12:31:15Z

lib/pure/collections/sequtils.nim

+    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


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

Yeah... this URL doesn't load.

dom96

Overall the PR is fine. If I wasn't so picky and unfamiliar with this code I would have merged it :)

dom96 · 2018-08-10T15:33:50Z

lib/pure/collections/sequtils.nim

+      # literal would be substituted twice
+      # BUG: `evalOnce(t, s, false)` would lead to C compile errors
+      # (`error: use of undeclared identifier`) instead of Nim compile errors
+      evalOnce(t, s, compiles((let _ = s)))


t, s

I know that you didn't introduce these names but I really despise single character variable names. In reviewing this PR I had to go back and forth between various templates to work out what t is meant to be here.

Perhaps they could be changed?

Side note: this evalOnce macro feels like a total hack to me.

The compiles check should be moved above in the enclosing if, you don't need the third parameter.

almost procs in this module use single char arg names so I don't wanna special case mapIt, feel free to change that in a subsequent PR.
change s to s2 to make it obvious it relates to s; also improved var names of evalOnce and added a bunch of documentation.

Side note: this evalOnce macro feels like a total hack to me.

feel free to improve that in a subsequent PR. It's there for a reason (avoid double evaluation bugs, handle openArray and other edge cases) and neither @LemonBoy nor I could come up with something better

dom96 · 2018-08-10T15:38:37Z

lib/pure/collections/sequtils.nim

+    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


Yeah... this URL doesn't load.

dom96 · 2018-08-10T15:40:06Z

lib/pure/collections/sequtils.nim

+
+  block: # mapIt with direct openArray
+    # see https://github.com/_render_node/MDExOlB1bGxSZXF1ZXN0MjA3MTQ3MTIw/pull_requests/timeline#issuecomment-411614711
+    doAssert openArray[int]([1,2]).mapIt(it) == @[1,2]


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.

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

dom96 · 2018-08-10T15:41:05Z

lib/pure/collections/sequtils.nim

+    doAssert openArray[int]([1,2]).mapIt(it) == @[1,2]
+
+  block: # mapIt test, see https://github.com/nim-lang/Nim/pull/8584#pullrequestreview-144723468
+    # NOTE: `[].mapIt(it)` is illegal, just as `let a = @[]` is


What about

mapIt([], it) mapIt(@[], it)

?

added a test doAssert: not compiles(mapIt(@[], it)) ; it should indeed not compile as explained in comments (lacks type information)

timotheecour · 2018-08-10T19:55:50Z

PTAL. all comments addressed.

LemonBoy · 2018-08-10T22:20:10Z

lib/pure/collections/sequtils.nim

+
+      # BUG: `evalOnceAs(s2, s, false)` would lead to C compile errors
+      # (`error: use of undeclared identifier`) instead of Nim compile errors
+      evalOnceAs(s2, s, compiles((let _ = s)))


This compiles(...) block should go with compiles(s.len).
If you don't do so and s is a openArray[int](...) the template expansion below will replicate s two times like this:

... var result = newSeq[outType](openArray[int](...).len) for it {.inject.} in openArray[int](...): ...

This edge case is better handled (IMO) by the other code path even though we don't make use of the .len, after all this is an edge case as you don't go and cast openarrays.

I know about this limitation (it's documented in the PR, see below) but I disagree with your proposed change:

with your suggestion, it'll affect valid (non edge case) code paths, resulting in un-optimized code for these.

please try the PR with your suggestion and see what happens by inserting these echo statements:

when compiles(s.len) and compiles((let _ = s)): block: evalOnceAs(s2, s, true) echo "optimized" var i = 0 var result = newSeq[outType](s2.len) for it {.inject.} in s2: result[i] = op i += 1 result else: echo "fallback" var result: seq[outType] = @[] for it {.inject.} in s: result.add(op) result

will print:

optimized fallback fallback fallback optimized optimized optimized optimized fallback

whereas with current PR it prints:

optimized optimized optimized optimized optimized optimized optimized optimized fallback

ie it does the correct thing (optimized code + no duplicate evaluation) in all cases except the in the very edge case of an openArray constructor passed to a proc (template would be ok).

copying verbatim from PR:

block: # mapIt with direct openArray proc foo1(x: openArray[int]): seq[int] = x.mapIt(it * 10) counter = 0 doAssert foo1(openArray[int]([identity(1),identity(2)])) == @[10,20] doAssert counter == 2 template foo2(x: openArray[int]): seq[int] = x.mapIt(it * 10) counter = 0 doAssert foo2(openArray[int]([identity(1),identity(2)])) == @[10,20] # TODO: this fails; not sure how to fix this case # doAssert counter == 2

Note also that this edge case behavior is pre-existing to this PR, so I see now reason we can't merge this now; we can always do a subsequent PR later if we find a way that doesn't affect non-edge cases adversly while handling this edge case.

Oh I see, that's fine by me then.

cool. PR still marked as @LemonBoy requested changes though...

timotheecour · 2018-08-14T00:13:47Z

PTAL: can we please get this merged ASAP? it fixes pre-existing bugs

Araq · 2018-08-14T08:44:09Z

it fixes pre-existing bugs

And we're sure it doesn't introduce new bugs, right?

Oh well, will clean this up later.

timotheecour · 2018-08-14T09:09:54Z

thank you.

And we're sure it doesn't introduce new bugs, right?

obviously no such guarantees, but it came with increased test coverage

…ice (#8586) * cleanups refs #8584 * fixes #7187

…rator twice (nim-lang#8586) * cleanups refs nim-lang#8584 * fixes nim-lang#7187

fixes nim-lang#8577, nim-lang#8580, other bug fixes

263a5c5

timotheecour mentioned this pull request Aug 9, 2018

Yet another change to evalOnce #8578

Closed

fix type=>typedesc for bootstrap support

f3cfd04

timotheecour force-pushed the exp_mapIt_simple branch from daf06f6 to f3cfd04 Compare August 9, 2018 05:41

timotheecour mentioned this pull request Aug 9, 2018

fixes #7187 sequtils.toSeq produces the sequence from the iterator twice #8586

Merged

LemonBoy suggested changes Aug 9, 2018

View reviewed changes

address comments

385be76

timotheecour force-pushed the exp_mapIt_simple branch from 89b36af to 385be76 Compare August 9, 2018 10:00

kaushalmodi reviewed Aug 10, 2018

View reviewed changes

dom96 approved these changes Aug 10, 2018

View reviewed changes

address comments

a5d9f15

LemonBoy reviewed Aug 10, 2018

View reviewed changes

LemonBoy approved these changes Aug 10, 2018

View reviewed changes

Araq merged commit ed9fd2b into nim-lang:devel Aug 14, 2018

timotheecour deleted the exp_mapIt_simple branch August 14, 2018 09:12

timotheecour added a commit to timotheecour/Nim that referenced this pull request Aug 16, 2018

cleanups post nim-lang#8584

a8a6191

Araq pushed a commit that referenced this pull request Aug 16, 2018

fixes #7187 sequtils.toSeq produces the sequence from the iterator tw…

af03754

…ice (#8586) * cleanups refs #8584 * fixes #7187

PMunch pushed a commit to PMunch/Nim that referenced this pull request Aug 17, 2018

fixes nim-lang#7187 sequtils.toSeq produces the sequence from the ite…

c4c80d8

…rator twice (nim-lang#8586) * cleanups refs nim-lang#8584 * fixes nim-lang#7187

timotheecour mentioned this pull request Mar 23, 2020

refactoring and yak shaving. #13687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timotheecour commented Aug 9, 2018 •

edited

Loading

LemonBoy left a comment

LemonBoy Aug 9, 2018

LemonBoy Aug 9, 2018

Araq Aug 9, 2018

LemonBoy Aug 9, 2018

timotheecour Aug 9, 2018

timotheecour Aug 9, 2018

timotheecour commented Aug 9, 2018

timotheecour commented Aug 9, 2018

kaushalmodi Aug 10, 2018

dom96 Aug 10, 2018

timotheecour Aug 10, 2018

dom96 left a comment •

edited

Loading

dom96 Aug 10, 2018

dom96 Aug 10, 2018

LemonBoy Aug 10, 2018

timotheecour Aug 10, 2018

dom96 Aug 10, 2018

dom96 Aug 10, 2018

timotheecour Aug 10, 2018

dom96 Aug 10, 2018

timotheecour Aug 10, 2018

timotheecour commented Aug 10, 2018

LemonBoy Aug 10, 2018

timotheecour Aug 10, 2018 •

edited

Loading

LemonBoy Aug 10, 2018

timotheecour Aug 10, 2018

timotheecour commented Aug 14, 2018

Araq commented Aug 14, 2018

timotheecour commented Aug 14, 2018

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

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

Conversation

timotheecour commented Aug 9, 2018 • edited Loading

LemonBoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 9, 2018

timotheecour commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dom96 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 10, 2018

Choose a reason for hiding this comment

timotheecour Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 14, 2018

Araq commented Aug 14, 2018

timotheecour commented Aug 14, 2018

timotheecour commented Aug 9, 2018 •

edited

Loading

dom96 left a comment •

edited

Loading

timotheecour Aug 10, 2018 •

edited

Loading