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

feature: define variadic function parser #34

Closed
wants to merge 7 commits into from

Conversation

kolharsam
Copy link

@kolharsam kolharsam commented May 11, 2020

Adds mainly the parser for the variadic function some of the other commits were already merged, I just forgot to merge your master prior to making these updates.

@kolharsam kolharsam marked this pull request as ready for review May 11, 2020 18:55
@jaseemabid
Copy link
Owner

Thanks! This looks good to me.

The PR includes 6 commits, 2 of which are already on master and one is a merge. Do you mind rebasing your branch on top of current master? That should give you the same result with just the 3 new commits you added for this change.

let (i, _) = tuple((open, tag("define"), space1))(i)?;
let (i, mut params) = delimited(open, identifiers, tag("."))(i)?;
let (i, rest_param) = delimited(space0, identifier, close)(i)?;
let (i, body) = delimited(space0, many1(terminated(expression, space0)), space0)(i)?;
Copy link
Owner

Choose a reason for hiding this comment

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

You might need space1 instead of space0 in let (i, rest_param) = delimited(space0, identifier, close)(i)?; to make sure that there is atleast one space after the ..

I havent tested this yet, but could you try what happens for (define (add x y .args) (reduce + 0 args)) please?

Copy link
Owner

Choose a reason for hiding this comment

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

image

The space is necessary, otherwise .args becomes one identifier instead of what we want here.

@kolharsam
Copy link
Author

Thanks! This looks good to me.

The PR includes 6 commits, 2 of which are already on master and one is a merge. Do you mind rebasing your branch on top of current master? That should give you the same result with just the 3 new commits you added for this change.

Right. Yes!

This change was done to make sure that we have one space
delimiter for sure - as mentioned in the review
@kolharsam kolharsam marked this pull request as draft May 12, 2020 11:54
@kolharsam
Copy link
Author

I'm closing this and will make a new PR after fixing those commits from my master.

@kolharsam kolharsam closed this May 12, 2020
@jaseemabid
Copy link
Owner

@kolharsam You could force push the rebased branch instead of reopening a new PR, since that keeps all the discussion in one place. Its not an issue in this case, but its a useful thing to know since its really not uncommon having to clean up a branch several times before getting it merged in a larger project.

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.

2 participants