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

vector macro patterns #272

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

Conversation

jrvidal
Copy link
Contributor

@jrvidal jrvidal commented Oct 9, 2024

Adds vector patterns to macros.

@@ -131,11 +131,7 @@ pub trait Folder {
}

#[inline]
fn visit_vector(&mut self, mut v: Vector) -> ExprKind {
let args: Vec<_> = v.args.into_iter().map(|exp| self.visit(exp)).collect();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've revisited a few of the visitor traits, trying to ascertain whether it makes sense or not to descend into the vector contents (given that they're self-quoting).

I don't have a lot of confidence in the end result, given that different visitors are used at different "expansion phases", and my understanding of those is not great.

Copy link
Owner

Choose a reason for hiding this comment

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

Given that they're self quoting, I think it is correct to not descend into the vector contents

return false;
};

if !match_list_pattern(subpatterns, &l.args, l.improper) {
// FIXME: this is not quite correct. It does not handle
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to fix this in this PR, but it's already sizeable and near overflowing my head capacity at the moment.

@@ -1445,6 +1445,12 @@ impl<'a> RecursiveEqualityHandler<'a> {
}
continue;
}
(MutFunc(l), MutFunc(r)) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated bugfix

@@ -90,13 +90,16 @@
[(quasiquote (#%unquote x)) x]

[(quasiquote ((#%unquote-splicing x))) (append x '())]
[(quasiquote #((#%unquote-splicing x))) (list->vector (append x '()))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very blunt and inefficient approach. Not sure if the vector builtins are/could be made available.

Copy link
Owner

Choose a reason for hiding this comment

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

We probably could - I think they should be available

That being said quasi quote is some of the most mind bending stuff to work on, so I'm fine with this for now and we can come back to it at some point in the future

@@ -1037,12 +1037,6 @@ impl<'a> Parser<'a> {
&self.source_name.clone(),
));

if matches!(current_frame.paren_mod, Some(ParenMod::Bytes))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bugfix: this logic was not exhaustive enough.

Copy link
Owner

@mattwparas mattwparas left a comment

Choose a reason for hiding this comment

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

This LGTM - there have been quite a few changes since this was opened, might be worth a rebase to make sure everything still is okay

@mattwparas
Copy link
Owner

Still interested in getting this merged?

@jrvidal
Copy link
Contributor Author

jrvidal commented Nov 24, 2024

Still interested in getting this merged?

I am, I was sidetracked by real life™️. I'll try to rebase it soon.

@mattwparas
Copy link
Owner

Still interested in getting this merged?

I am, I was sidetracked by real life™️. I'll try to rebase it soon.

No problem at all 😄 (also, no rush!)

Copy link

@Haitrang99 Haitrang99 left a comment

Choose a reason for hiding this comment

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

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.

3 participants