-
Notifications
You must be signed in to change notification settings - Fork 13k
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
remove erroneous error message when checking impl trait params #48709
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
e848adf
to
a951343
Compare
i don't really like my solution here, but idk if doing the test in both functions would be any better |
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.
Seems good to me, the only nit is in the name of the parameter, which could be clearer I think.
src/librustc_typeck/check/mod.rs
Outdated
@@ -4919,7 +4919,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
fn check_path_parameter_count(&self, | |||
span: Span, | |||
segment: &mut Option<(&hir::PathSegment, &ty::Generics)>, | |||
is_method_call: bool) { | |||
is_method_call: bool, | |||
is_impl_trait: bool) { |
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.
Nit: lots of bool parameters can get sorta hard to read. I like to do:
struct IsMethodCall(bool);
struct IsImplTrait(bool);
and then use those types instead of plain bool. Then the caller can do check_path_parameter_count(span, segment, IsMethodCall(true), IsImplTrait(false))
or whatever. Inside the function, you can read the value by doing is_method_call.0
.
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.
But in any case I kind of think the name is_impl_trait
is not very informative. How about suppress_mismatch_error
or something?
@tinaun -- do you think you'll have a chance to address the nit above? ❤️ |
6e23c12
to
d7dfc85
Compare
@nikomatsakis looks like there has been new code pushed! |
True, although it did not address my nit. That's ok, I don't care that much, but @tinaun you will need to fix this travis error: https://travis-ci.org/rust-lang/rust/builds/353614751#L1817 r=me once green |
@tinaun ping from triage! The travis build is failing, could you fix that so the PR can be merged? |
@bors r+ |
📌 Commit 97e0dc3 has been approved by |
remove erroneous error message when checking impl trait params fixes rust-lang#48703
⌛ Testing commit 97e0dc3 with merge 290420ba65d7e5829826c9cd84d20d9a923e3138... |
@bors: retry prioritizing rollup with this included in it |
remove erroneous error message when checking impl trait params fixes #48703
☀️ Test successful - status-appveyor, status-travis |
fixes #48703