-
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
correctly lower impl const
to bind to host effect param
#114545
Conversation
impl const
to bind to host effect paramimpl const
to bind to host effect param
This comment has been minimized.
This comment has been minimized.
30d5d1d
to
6c1e3bb
Compare
This comment has been minimized.
This comment has been minimized.
if !infer_args && has_default { | ||
if !infer_args | ||
&& has_default | ||
&& !tcx.has_attr(param.def_id, sym::rustc_host) |
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.
How does this work for desugared params?
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.
desugared params get this attribute as well: link
r=me with tidy happy |
a review of this commit which fixes the ICE would be nice |
let mut path_segments = ast_trait_ref.path.segments.to_vec(); | ||
let last_segment = path_segments.len() - 1; | ||
let mut args = path_segments[last_segment].args().clone(); | ||
args.args = &args.args[..args.args.len() - 1]; |
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.
maybe assert that you are actually removing the host
param, and not something else that snuck in
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 turns out to be a bit more complicated and I didn't want to look inside bodies, so I just added the attr to all automatically generated bodies and asserted that the attr exists instead.
8d91309
to
057be38
Compare
@bors r+ |
⌛ Testing commit 057be38 with merge 6069c503450c448e96bd0ebbd98fcfb5087b3304... |
💔 Test failed - checks-actions |
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot)
|
spurious @bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (f88a8b7): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
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. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.146s -> 632.905s (0.12%) |
r? @oli-obk
cc #110395