-
Notifications
You must be signed in to change notification settings - Fork 323
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
Further simplification enabled by chained conversions #11682
Conversation
92636c3
to
932d2f4
Compare
@@ -284,34 +281,13 @@ pub enum Precedence { | |||
Negation, | |||
Application, | |||
Annotation, | |||
/// A value that is higher than the precedence of any operator. | |||
Max, | |||
// NOTE: The highest value must not exceed 0x7e--see usage of `into_u8`. |
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 see only one usage of into_u8: in ModifiedPrecedence::new
; there's no docs, and I don't know why 0x7f is not allowed.
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.
It's unlikely we would ever want to use high numeric values for these, but out of an abundance of caution I put a note here to make sure that anyone considering doing so knows what that could affect. If they are really determined to make that sort of change, the reference to into_u8
points them in the right direction to find out why they can't use 0x7f.
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.
Have I provided a useful review for Rust code!?
😆 This isn't the first time you've made some good points--at this point you're a bona fide Rustacean. As a Rust developer, when you have to work in Java do you miss the zero-cost abstractions? |
Pull Request Description
Some simplification enabled by the refactor in #11671. Inspired by review from @JaroslavTulach.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.