-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Cut some needless muts in compiler #145639
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
Conversation
|
cc @Muscraft
cc @Muscraft Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs cc @ZuseZ4 Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in match lowering cc @Nadrieril Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri changes to the core type system Some changes occurred in coverage instrumentation. cc @Zalathar Some changes occurred to the CTFE machinery Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in cc @BoxyUwU Some changes occurred in need_type_info.rs cc @lcnr Some changes occurred in match checking cc @Nadrieril Some changes occurred to constck cc @fee1-dead |
| /// If this returns `None`, the function call has been handled and the function has returned. | ||
| fn hook_special_const_fn( | ||
| &mut self, | ||
| &self, |
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.
Please keep the mut here, we don't know which functions will be hooked here in the future and they may need mutable access.
lcnr
left a comment
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.
While I think some of these changes are desirable and cleanup the code and it's helpful to state "this doesn't actually mutate the visitor", there are quite a few cases where whether they mutate some context is an implementation detail and the fact that they currently don't isn't actually meaningful and would result in churn.
e.g. in confirmation all confirm_X functions should all be able to mutate the SelectionContext and the fact that they don't right now isn't meaningful. If we change them in the future to do so we now need to know whether adding mut here is fine, readd it, and then hope that nobody used them in ways where that's annoying.
I don't think removing them based on a mechanical check for "is the mut currently required" is worth it. There are a few which seem like clear improvements, but a general blanket change feels ambiguous (and likely to cause merge conflicts) enough that I would prefer not merging this.
Hello. This PR cuts most of the unnecessary
&mutreferences from the compiler, changing them to plain&references instead. There are also a few resultingmutvariable flags cut as well.Should cause no change in behavior. Possibly will make something along the way a little faster?
x checkandx clippypassed locally.