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

Better handling of comments #483

Merged
merged 10 commits into from
Sep 26, 2024
Merged

Better handling of comments #483

merged 10 commits into from
Sep 26, 2024

Conversation

msprotz
Copy link
Contributor

@msprotz msprotz commented Sep 13, 2024

The goal of this PR is to move the handling of in-expression comments from dedicated AST nodes to meta-level attributes on nodes. The reason for this is that the EComment node potentially trips a lot of transformation passes that perform large pattern-matches on AST nodes that may start failing if an EComment node is found in the wrong place.

Rather than add an (expensive) strip_comments function everywhere, I chose instead to move the comments to meta-level attributes on nodes themselves, which I believe makes things much easier to handle.

The problem is that this, in turn, drops comments unless I audit every single pass to make sure it doesn't fill out an empty default meta... so I'm not sure this is worth merging..

@msprotz msprotz marked this pull request as ready for review September 24, 2024 18:33
@msprotz
Copy link
Contributor Author

msprotz commented Sep 24, 2024

After carefully reviewing our options here, I think this is the best bet: I'd rather get an annoying, but not life-threatening issue where comments are dropped, rather than a semantically meaningful phase that silently stops operating because of an EComment node in the middle of a term that prevents a pattern from matching.

@tahina-pro you are the biggest user of the "inline comment" feature, which I attested still works (strengthened test forthcoming) -- unless I hear from you within the next couple days, I'll just merge this and assume that everparse is not affected, thank you

@msprotz
Copy link
Contributor Author

msprotz commented Sep 24, 2024

CC @Nadrieril since we've been chatting about this, the goal being to successfully propagate comments from Rust to C in Eurydice

msprotz added a commit to AeneasVerif/eurydice that referenced this pull request Sep 24, 2024
@msprotz msprotz merged commit 8c36120 into master Sep 26, 2024
2 checks passed
@msprotz msprotz deleted the protz_comments branch September 26, 2024 18:08
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.

1 participant