Skip to content

Fix valid-let-bindings? #629

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

Merged
merged 1 commit into from
May 7, 2024
Merged

Conversation

jpellegrini
Copy link
Contributor

We had this:

(let 1 2)
=> #[primitive %execute]

because valid-let-bindings? would return #f and not trigger an error when bindings was not a list.

This patch fixes that error, and passes all tests.

We had this:
```
(let 1 2)
=> #[primitive %execute]
```
because `valid-let-bindings?` would return #f and not trigger
an error when `bindings` was not a list.

This patch fixes that error, and passes all tests.
@jpellegrini
Copy link
Contributor Author

By the way, I'm also working on SRFI 71 (Extended LET-syntax for multiple values), and that's how I found this problem.
71 is not a popular SRFI, and I know we already have let-values, but... It's formally a standard, and actually not that hard to implement. :)

@jpellegrini
Copy link
Contributor Author

By the way, I didn't include tests because the error raised cannot be caught... It would break the test procedure.
Is there a way to catch any possible error, including those during compilation?

@jpellegrini
Copy link
Contributor Author

Perhaps it would also make sense to change the procedure name to check-let-bindings, since after this patch it would not be a predicate anymore?

@egallesio egallesio merged commit a2bad2c into egallesio:master May 7, 2024
@egallesio
Copy link
Owner

Thanks @jpellegrini, for this PR.

Is there a way to catch any possible error, including those during compilation?

Yes, use test/compile-error.

Perhaps it would also make sense to change the procedure name to check-let-bindings, since after this patch it would not be a predicate anymore?

You are right. Done.

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