[CSSolver] Refactor selectBestBindingDisjunction
#18785
Conversation
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
Build failed |
|
|
|
Looks like it's going to be resolved by #18797 |
lib/Sema/Constraint.cpp
Outdated
| Type commonType; | ||
| assert(llvm::all_of(constraints, [&](const Constraint *choice) -> bool { | ||
| auto currentType = choice->getFirstType()->lookThroughAllOptionalTypes(); | ||
| assert(currentType); |
There was a problem hiding this comment.
This assert doesn't seem all that useful since we would crash in lookThroughAllOptionalTypes() if getFirstType() returned false and lookThroughAllOptionalTypes() never returns nullptr (and it would break code all over if that changed).
There was a problem hiding this comment.
Ah, right, just went overly paranoid... :)
There was a problem hiding this comment.
Removed assert and made it so we skip disjunctions formed by restrictions/fixes, this way we don't have to lookThroughAllOptionalTypes
lib/Sema/Constraint.cpp
Outdated
|
|
||
| return commonType->isEqual(currentType); | ||
| })); | ||
| (void)commonType; |
There was a problem hiding this comment.
You shouldn't need this as the declaration is under #ifndef.
xedin
force-pushed
the
select-best-disjunction-improvements
branch
from
acb94b2 to
838f87f
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
lib/Sema/CSSolver.cpp
Outdated
| // Only do this for simple type variable bindings, not for | ||
| // bindings like: ($T1) -> $T2 bind String -> Int | ||
| if (auto *typeVar = getAsTypeVar(choice->getFirstType())) | ||
| bindingDisjunctions.push_back({disjunction, typeVar}); |
There was a problem hiding this comment.
As mentioned in the other review thread we should be able to just immediately gather the constraints for the type variable and do the test below rather than collecting all the disjunctions and then going back through them and gathering constraints.
There was a problem hiding this comment.
Ah sure, I can do that too! :)
There was a problem hiding this comment.
Just didn't want to change the shape too much...
There was a problem hiding this comment.
Alright, new changes are in!
xedin
force-pushed
the
select-best-disjunction-improvements
branch
from
838f87f to
96b7bc8
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
lib/Sema/CSSolver.cpp
Outdated
| ->getRValueType() | ||
| ->getAs<TypeVariableType>(); | ||
| assert(tv); | ||
| bindingDisjunctions.push_back(disjunction); |
There was a problem hiding this comment.
This no longer needs to be a vector. You should just be able to keep track of the fact that you saw some binding disjunction and return it.
There was a problem hiding this comment.
Fixed! This is now net-negative locs :)
Add an assert which verifies that all of the disjunction choices has the same type variable on the left-hand side. Based on that fact, refactor `selectBestBindingDisjunction` to only check first choice to find suitable "bind" disjunctions to favor.
xedin
force-pushed
the
select-best-disjunction-improvements
branch
from
96b7bc8 to
e81ce93
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test source compatibility |
Add an assert which verifies that all of the disjunction
choices has the same type variable on the left-hand side.
Based on that fact, refactor
selectBestBindingDisjunctionto only check first choice to find suitable "bind" disjunctions
to favor.