-
Notifications
You must be signed in to change notification settings - Fork 23
Some small patch-ups around matching on extensions #140
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about this. On the one hand, I like it a lot -- saying "use get_desc
instead of .foo_desc
" is genuinely great. On the other hand, I worry about diffs with upstream. One careful design decision around of_ast
was that we didn't need to edit the existing matches at all. We now have to change function preambles in terms of what they bring in to scope, edit what matches match on, and so forth. These may mostly be resolvable automatically, but I have some concerns about that.
You're right. I've mostly reverted. I didn't quite catch the main payload of the big comments as "this avoids merge conflicts". I've revised to make this clearer. Please re-review. Thanks! |
(After this is reviewed, I'll rebase the rest of the chain.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Richard, trying to make the comment highlight that more is a good plan. I think this is mostly good now, I just had a couple minor notes around comment wording. We should also rename this PR now that what it does is different.
I think you made the right call by reverting, but it's not obvious; there were wins in the get_desc
design!
The goal here is to more structurally avoid the possibility of forgetting about extensions. This doesn't really actually get us there, though, because we can't change Parsetree to avoid using e.g. pexp_desc. And even if we could make that change, we might not want to, because it's just too convenient to just use pexp_desc. Until we have active patterns of some sort, we may be unable to improve upon the status quo. However, as I was working this, I identified a few spots where I think there are live bugs for lack of looking for extensions. And I think having a destructor, as this commit adds, is a positive little improvement.
f091984
to
2f9d1a3
Compare
EDIT: This was originally a much more ambitious patch, but has been scaled down because the original design would make upstreaming harder.
The goal here is to more structurally avoid the possibility of
forgetting about extensions. This doesn't really actually get us
there, though, because we can't change Parsetree to avoid using
e.g. pexp_desc. And even if we could make that change, we might
not want to, because it's just too convenient to just use pexp_desc.
Until we have active patterns of some sort, we may be unable to
improve upon the status quo.
However, as I was working this, I identified a few spots where I
think there are live bugs for lack of looking for extensions. And
I think having a destructor, as this commit adds, is a positive little
improvement.
This PR is against the
rae/no-raw-type
branch, because I'm stacking PRs.