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

Reader Macro Expansion Test Step 8 #564

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wgmyers
Copy link

@wgmyers wgmyers commented May 29, 2021

Pull request added per issue #560.

Test has been added to step 8 rather than step 7 in order to be able to use defmacro!.

My first pull request here: apologies in advance if I've done anything wrong: let me know and I'll try and fix it.

@asarhaddon
Copy link
Contributor

Testing the reader in step1 is easyer because no evaluation is involved yet.

`(a (b) c)
;=> (quasiquote (a (b) c))

The faulty reader (in the commit before the fix referenced in issue #560) answers (quasiquote (a (b)) c). The same probably holds for other reader macros.

@wgmyers
Copy link
Author

wgmyers commented Jun 1, 2021

I don't know what the criteria are for adding tests, so I'm not sure how to answer here.

It's absolutely true that the suggested step 1 test above would catch the exact implementation bug that I managed to perpetrate - my code was indeed over-aggressively closing brackets before it should have done, hence managing to turn `(a (b) c) into (quasiquote (a (b)) c) rather than (quasiquote (a (b) c)). However, this suggested test would only catch that and not other potential subtle misimplementations.

The test I have suggested in this PR would catch a wider range of potential implementation bugs: rather than testing for a specific implementation error it instead tests for being able to successfully decompose a complex expression with multiple instances of reader macros in the correct way.

Either would have saved me from getting to the end of step A without being able to run make "perf^mal", and I can see arguments for both tests being used: the step 1 test suggested above tests specifically for the exact bug we have found in the wild as perpetrated by me; the step 8 test in my PR tests for the whole class of such bugs, or at least some hopefully large subset of it.

@asarhaddon
Copy link
Contributor

Investigating macro and quasiquote expansion is difficult. I will not discuss the large test, but the short test should definitely be included in step1 for each reader macro.

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