Typecheck refactor for !
#35787
Typecheck refactor for !
#35787
Conversation
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -38,12 +38,11 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
| self.write_ty(pat.id, expected); | |||
| } | |||
| PatKind::Lit(ref lt) => { | |||
| self.check_expr(<); | |||
| let expr_ty = self.expr_ty(<); | |||
| let expr_t = self.check_expr(<); | |||
There was a problem hiding this comment.
Why this change? expr_ty is a better name for such a variable.
There was a problem hiding this comment.
Because having a method named expr_ty and a variable named expr_ty is confusing.
There was a problem hiding this comment.
I find _t worse. Besides, why do we still need the expr_ty method?
There was a problem hiding this comment.
expr_ty is still used in a few places. I can keep going trying to remove it entirely if you like. Otherwise I can rename the variable ty.
|
I would move the match in |
| } | ||
| hir::ExprBlock(ref b) => { | ||
| self.check_block_with_expected(&b, expected); | ||
| self.write_ty(id, self.node_ty(b.id)); | ||
| self.node_ty(b.id) |
There was a problem hiding this comment.
check_block_with_expected should return the type.
|
@eddyb is this PR good to go? I can do the same thing with patterns (factoring out |
|
@canndrew Yeah, doing that would be great. Are there many |
|
☔ The latest upstream changes (presumably #36030) made this pull request unmergeable. Please resolve the merge conflicts. |
canndrew
force-pushed
the
bang_type_refactor_check
branch
from
a5c390f to
234939a
Compare
|
I've done the factoring-out for patterns. There's still some uses of |
|
@canndrew Can't both of those takes the types in? Although not confirm.rs if it's a late adjustment. |
|
I've made the coercions code take the types as arguments. The code in confirm.rs walks over a big expression tree reading the types using |
| @@ -414,11 +414,14 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
| } | |||
| _ => result_ty | |||
| }; | |||
|
|
|||
| let mut arm_tys = Vec::new(); | |||
There was a problem hiding this comment.
This shouldn't be needed though. The design was such that no extra bookkeeping was needed.
|
Any idea what these LLVM linker errors are about? |
|
☔ The latest upstream changes (presumably #36016) made this pull request unmergeable. Please resolve the merge conflicts. |
canndrew
force-pushed
the
bang_type_refactor_check
branch
from
12e74d8 to
c9a340e
Compare
|
@eddyb I rebased this and now it's passing. Is it good to merge? |
|
@bors r+ Thanks! |
|
📌 Commit c9a340e has been approved by |
Typecheck refactor for `!` Ping @nikomatsakis @eddyb. This is the PR for the typeck refactor for `!`. Is this what you guys had in mind? Is there anything else that needs doing on it?
Ping @nikomatsakis @eddyb. This is the PR for the typeck refactor for
!. Is this what you guys had in mind? Is there anything else that needs doing on it?