Skip to content

cps: experimental support for for-loops#84

Draft
alaviss wants to merge 1 commit into
nim-works:masterfrom
alaviss:for-loops
Draft

cps: experimental support for for-loops#84
alaviss wants to merge 1 commit into
nim-works:masterfrom
alaviss:for-loops

Conversation

@alaviss

@alaviss alaviss commented May 3, 2021

Copy link
Copy Markdown
Contributor

For-loops are inlined using block statements, which messes with their
control flow due to #76.

Breaks "for loop with continue, break" test. On the flip side,
splitting now functions on them.

Also breaks a test in tzevv due to defer being rewritten into
try-finally, which doesn't work due to #80.

@disruptek disruptek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! 👍

Comment thread cps.nim Outdated
n[1] = getTypeInst n[2]
else:
# get the type from the symbol as the last resort.
# walkaround for #48.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still workaround

Comment thread cps.nim

macro cps*(T: typed, n: typed): untyped =
macro cps2(T: typed, n: typed): untyped =
# I hate doing stuff inside macros, call the proc to do the work

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# I hate doing stuff inside macros, call the proc to do the work
## We used `getImplTransformed` to make a pre-pass whatfer transforming `for` and `defer`
## into the input to this macro, where we perform the meat of the CPS transform.

@saem saem May 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we could use a for loop macro to remove part of the getImplTransformed dependency, I still need to think about the defer one.

Not a blocker

Comment thread cps.nim
Comment on lines 875 to 949
when defined(nimdoc):
result = n
else:
result = cpsXfrm(T, n)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This when should move into cps().

Comment thread cps.nim
else:
result = cpsXfrm(T, n)

macro cps*(T: typed, n: typed): untyped =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
macro cps*(T: typed, n: typed): untyped =
macro cps*(T: typed, n: typed): untyped =
## Rewrite procedure `n` in Continuation-Passing Style, extending type `T` to store any transient locals.

@disruptek
disruptek requested a review from saem May 3, 2021 14:20
@disruptek disruptek linked an issue May 3, 2021 that may be closed by this pull request

@saem saem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Noted the for loop macro idea for posterity. @disruptek already highlighted the doc bits, lgtm.

For-loops are inlined using block statements, which messes with their
control flow due to nim-works#76.

Breaks a test in tzevv due to defer being rewritten into
try-finally, which doesn't work.

Co-authored-by: Andy Davidoff <github@andy.disruptek.com>
@alaviss

alaviss commented May 14, 2021

Copy link
Copy Markdown
Contributor Author

Rebased, compiler issues that I found:

  • case statement without else will have an else: nil appended:
case true
of true:
  echo "true"
of false:
  echo "failed"

transformed to:

case true
of true:
  echo "true"
of false:
  echo "failed"
else:
  nil

And this stuff don't pass sem, obviously.

  • the flattening of string concatenation:
a & b & c

transformed to

`&`(a, b, c)

And it also doesn't pass sem because a & with 3 arguments doesn't exist.

I'd say we should perform this transform ourselves.

@disruptek disruptek added the enhancement New feature or request label Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

for loops could be supported with a compiler fix

3 participants