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

Update pattern-matching.md #202

Merged
merged 3 commits into from
May 9, 2024
Merged

Update pattern-matching.md #202

merged 3 commits into from
May 9, 2024

Conversation

jkroso
Copy link
Contributor

@jkroso jkroso commented May 8, 2024

Explain why QuoteNode never matches

Explain why QuoteNode never matches
Copy link
Collaborator

@cstjean cstjean left a comment

Choose a reason for hiding this comment

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

Thank you for the documentation PR! Could you please post an example of the code? I don't quite get what you're referring to.

docs/src/pattern-matching.md Outdated Show resolved Hide resolved
Co-authored-by: Cédric St-Jean <cedric.stjean@gmail.com>
@jkroso
Copy link
Contributor Author

jkroso commented May 8, 2024

@match :(:a) begin
  s_QuoteNode => 1
  s_quote => 2
end

You might expect the first pattern to match but actually its the second one that does so the result of that expression is 2.

@cstjean
Copy link
Collaborator

cstjean commented May 8, 2024

I see! Perhaps that example would be good to add to the PR.

Do you think it'd be reasonable to fix it? We can merge this PR in the meantime if you don't have time to consider it, but special-casing QuoteNode in the macro code doesn't sound unreasonable to me.

@jkroso
Copy link
Contributor Author

jkroso commented May 8, 2024

If the Julia parser only produces QuoteNodes for quoted symbols then it would make sense to change the way MacroTools handles QuoteNodes. And as far as I know that's the case but I’m not sure on that. In a sense then though MacroTools currently does special case QuoteNode so making it work the way I expected would mean removing that special case. Which is just 1 line

@cstjean
Copy link
Collaborator

cstjean commented May 9, 2024

And that line comes from 1e01db9, 9 years ago, with not much to go on.

The problem with MacroTools is that A) it is lacking test cases B) breaking it can break a good chunk of the Julia ecosystem...

Ah, I'd lean towards merging this PR at the moment unless we are really confident of the implications of removing that line. It's more or less a breaking change. Can you please add the example you posted above?

@jkroso
Copy link
Contributor Author

jkroso commented May 9, 2024

Yeah I think the best we can do is document the odity for now. I reworded it to include the example. How does it read now?

@cstjean
Copy link
Collaborator

cstjean commented May 9, 2024

Perfect, thanks.

@cstjean cstjean merged commit 0daeb44 into FluxML:master May 9, 2024
11 checks passed
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.

None yet

2 participants