-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Properly handle postfix inc/dec in standalone and subexpr scenarios #104875
Conversation
…nd subexpr scenarios
let prev_is_semi = { | ||
if let Ok(prev_code) = self.sess.source_map().span_to_prev_source(lhs.span) && | ||
prev_code.trim_end().ends_with(";") { | ||
true | ||
} else { | ||
false | ||
} | ||
}; |
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.
This is a bit of a hack, but fair given where it lives. I would like to do a perf run of an alternative approach: modify lhs
to hold a starts_statement: bool
field. I think that it might hit memory a bit, but that would be far less brittle to refactors breaking this heuristic. Would you have time to try doing that as part of this PR?
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.
You means add a field with boolean type to LhsExpr
type?
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.
Updated the code.
I don't think there is a perf regerssion here, we are at error report codepath,
but you may have a double-check.
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.
Yes, maybe cost a little bit more memory.
--> $DIR/issue-104867-inc-dec-2.rs:18:18 | ||
| | ||
LL | let _ = i + i++; | ||
| ^^ not a valid postfix operator |
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.
This is the hardest case to handle well, so I removed the suggestion for this scenario.
There maybe a solution to get it right, I think it may not worth to do it.
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (309c469): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. |
…estebank Properly handle postfix inc/dec in standalone and subexpr scenarios Fixes rust-lang#104867 r? `@estebank`
Fixes #104867
r? @estebank