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

Fix token_to_integer to not require Copyable #670

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Destroyerrrocket
Copy link

We can simply use a reference

We can simply use a reference
Copy link
Collaborator

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This seems fine, but I'm curious why the call to sort was removed?

@@ -176,7 +176,7 @@ impl<'s> LowerState<'s> {

read_algorithm(&grammar.annotations, &mut algorithm);

let mut all_terminals: Vec<_> = self
let all_terminals: Vec<_> = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Author

Choose a reason for hiding this comment

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

mut was not necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

@Destroyerrrocket As I understand, he is asking about the removed sort below.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, that sort caused problems while trying to match str&. Tbf, it's been a while and I don't remember all the details, but it gave me problems...

@@ -19,7 +19,7 @@ macro_rules! return_err {
return Err(NormError {
message: format!($($args),+),
span: $span
});
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there a compatibility warning about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The original code makes warning. The changed one is fine.

@nikomatsakis nikomatsakis added this to the 1.0 milestone Mar 21, 2023
@Destroyerrrocket
Copy link
Author

This seems fine, but I'm curious why the call to sort was removed?

This way the order of the match arms is preserved, otherwise some arms that are more general would be moved over more specific ones.

@Destroyerrrocket
Copy link
Author

In this issue I explained in detail what that sort does:
#671

@Pat-Lafon
Copy link
Contributor

Hmmm, could we split off the Copy restriction from the sorting to unblock bf55006? That seems like something easy to merge on its own.

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

4 participants