Skip to content

Conversation

@coriolinus
Copy link
Member

Closes #1221.

Implements the approach suggested in that issue.

Closes #1221.

Implements the approach suggested in that issue.
@coriolinus coriolinus self-assigned this Apr 7, 2021
@coriolinus
Copy link
Member Author

Note that the current example solution suffers from the attack this PR is meant to ban. This means that we need to update the example.

I'd ideally use a student solution which is both compact and readable, but I'm not happy with the ethics of just grabbing it without permission, even with attribution. I think that approach is only viable if the student approves, and I don't have a good way to contact them. I don't think that mentor notes are the appropriate venue for such a request.

I'm happy to contribute my own solution, but that solution has not been reviewed at all and requires an external library. It's very readable (IMO), but I got a little architecture-happy with it.

@coriolinus coriolinus requested a review from a user April 7, 2021 23:10
@iHiD
Copy link
Member

iHiD commented Apr 7, 2021

We don't need permission. Everything a student uploads is licensed to us to use as we want. That's made very explicit during the signup process.

Once it's done though I can let them know :)

@coriolinus
Copy link
Member Author

Thanks @iHiD, I think they'd appreciate knowing that once this is merged.

@@ -0,0 +1,96 @@
//! Certain implementations of Forth naively and eagerly expand sub-definitions
Copy link

Choose a reason for hiding this comment

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

Love the added context here!

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.

[forth] Add test ruling out eager expansion of custom arguments

3 participants