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

Foreach in sketch #8141

Closed
wants to merge 6 commits into from
Closed

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Jul 31, 2013

This is a preliminary implementation of for ... in ... { ...} using a transitionary keyword foreach. Codesize seems to be a little bit down (10% or less non-opt) and otherwise it seems quite trivial to rewrite lambda-based loops to use it. Once we've rewritten the codebase away from lambda-based for we can retarget that word at the same production, snapshot, rewrite the keywords in one go, and expire foreach.

Feedback welcome. It's a desugaring-based approach which is arguably something we should have been doing for other constructs before. I apologize both for the laziness associated with doing it this way and with any sense that I'm bending rules I put in place previously concerning "never doing desugarings". I put the expansion in expand.rs and would be amenable to the argument that the code there needs better factoring / more helpers / to move to a submodule or helper function. It does seem to work at this point, though, and I gather we'd like to get the shift done relatively quickly.

@brson
Copy link
Contributor

brson commented Jul 31, 2013

Desugaring is definitely the right approach here - it's awesome how low-impact this is. I imagine the error messages could get troublesome though.

With the desugaring strategy how to we make for work for both Iterator and Iterable? I'm imagining we'll just have to implement one in terms of the other.

@huonw
Copy link
Member

huonw commented Jul 31, 2013

Fwiw, there's a pile of AST building methods in ext/build.rs, and so the mk_* functions are probably unnecessary (and most of the direct construction of ast structs/enums).

bors added a commit that referenced this pull request Jul 31, 2013
This is a preliminary implementation of `for ... in ... { ...}` using a transitionary keyword `foreach`. Codesize seems to be a little bit down (10% or less non-opt) and otherwise it seems quite trivial to rewrite lambda-based loops to use it. Once we've rewritten the codebase away from lambda-based `for` we can retarget that word at the same production, snapshot, rewrite the keywords in one go, and expire `foreach`.

Feedback welcome. It's a desugaring-based approach which is arguably something we should have been doing for other constructs before. I apologize both for the laziness associated with doing it this way and with any sense that I'm bending rules I put in place previously concerning "never doing desugarings". I put the expansion in `expand.rs` and would be amenable to the argument that the code there needs better factoring / more helpers / to move to a submodule or helper function. It does seem to work at this point, though, and I gather we'd like to get the shift done relatively quickly.
@bors bors closed this Jul 31, 2013
@graydon
Copy link
Contributor Author

graydon commented Jul 31, 2013

@brson to work with Iterator and Iterable I imagine it'll suffice to take the reference to the result of a call like let _i = &mut to_iter(<iter_expr>) or perhaps .iter(), assuming we implement it on Iterator to just return self. I wasn't sure we wanted to do that yet, but it should be backwards compatible to enhance for that in the future.

@bluss
Copy link
Member

bluss commented Jul 31, 2013

The bug #7597 is tracking Iterable. Compiler bug prevents it at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants