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

Use upstream's Sprite annotations (/*@...*/) #266

Open
wants to merge 3 commits into
base: pure-z3
Choose a base branch
from

Conversation

janvrany
Copy link
Collaborator

This PR scratches the worst of my itches when writing Sprite by hand. Still, there's more to fix (#193 and things I already forget)

This makes it easy to cross-check examples with upstream Sprite.
@janvrany janvrany requested a review from shingarov April 30, 2024 18:48
Copy link
Owner

@shingarov shingarov left a comment

Choose a reason for hiding this comment

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

If we are already here, focusing our attention on properly parsing the upstream /*@...*/ annotation syntax, why not go all the way and remove ⟦...⟧, (changing the tests of course), this way we are char-for-char identical to the upstream? I recommend focusing on one topic per PR. This particular change would be named something like "Use upstream Sprite annotation syntax (/@…/)", and cherrypick this commit, change the tests, and remove the ⟦...⟧.

Copy link
Owner

@shingarov shingarov left a comment

Choose a reason for hiding this comment

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

I don't get it. Are you suggesing to depart from the upstream syntax here?

In upstream, if an id starts with uppercase, you know it's either a data constructor (see how ctorP calls cbind in Parse.hs), or a switch-case (altP in Parse.hs).

Copy link
Owner

@shingarov shingarov left a comment

Choose a reason for hiding this comment

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

Wait, how is this any different? I am looking at #word and it is PPCharSetPredicate on isAlphaNumeric, which is just isLetter or: [isDigit], no?

@janvrany
Copy link
Collaborator Author

janvrany commented May 1, 2024

I recommend focusing on one topic per PR. This particular change would be named something like "Use upstream Sprite annotation syntax (/@…/)", and cherrypick this commit, change the tests, and remove the ⟦...⟧.

OK, no problem.

Are you suggesing to depart from the upstream syntax here?

Yes.

Wait, how is this any different? I am looking at #word...

Ah, my bad. IIRC this has not been so in the early versions of PP, will drop that change.

@janvrany janvrany force-pushed the pr/improve-sprite-parser-1 branch from 06a90d6 to 15e5aab Compare May 1, 2024 14:22
@janvrany janvrany changed the title Improve sprite parser Use upstream's Sprite annotations (/*@...*/) May 1, 2024
@janvrany
Copy link
Collaborator Author

janvrany commented May 1, 2024

Are you suggesing to depart from the upstream syntax here?

Forget it, I'll deal with that differently some time in the future.

@shingarov
Copy link
Owner

Ok, this is excellent.

Now the next question is how to merge such things. Because if we merge into pure-z3, it will break all the branches. I think this simply means we should prioritize merging that whole zoo-of-branches into pure-z3 asap.

@janvrany
Copy link
Collaborator Author

janvrany commented May 3, 2024

We can drop last commit (removing support for [[ ]]) and add it later, once the rest is merged, or the other way round, Up to you.

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