Prevent unnecessary work in IntersectionType#5054
Conversation
|
This pull request has been marked as ready for review. |
VincentLanglet
left a comment
There was a problem hiding this comment.
TrinaryLogic has lazyMaxMin, isn't the right moment to introduce IsSuperTypeOfResult::lazy method ? (And maybe the same for AcceptsResult ?)
good idea, adjusted accordingly. |
src/Type/AcceptsResult.php
Outdated
| $results = []; | ||
| foreach ($objects as $object) { | ||
| $isAcceptedBy = $callback($object); | ||
| if ($isAcceptedBy->result->yes()) { | ||
| return $isAcceptedBy; | ||
| } | ||
| $results[] = $isAcceptedBy; | ||
| } | ||
| return self::maxMin(...$results); |
There was a problem hiding this comment.
This add another foreach.
Looking at TrinaryLogic::lazyMaxMin it does not call self::maxMin.
Should we implement it this way
public static function lazyMaxMin(
array $objects,
callable $callback,
): self
{
if ($objects === []) {
throw new ShouldNotHappenException();
}
$results = [];
$reasons = [];
foreach ($objects as $object) {
$isAcceptedBy = $callback($object);
if ($isAcceptedBy->result->yes()) {
return $isAcceptedBy;
}
$results[] = $isAcceptedBy->result;
foreach ($isAcceptedBy->reasons as $reason) {
$reasons[] = $reason;
}
}
return new self(TrinaryLogic::maxMin(...$results), array_values(array_unique($reasons)));
}
instead ?
(I also wonder if we should -- now or later -- avoid calling TrinaryLogic::maxMin to remove an extra foreach and doing it in the existing one)
There was a problem hiding this comment.
ok - unrolled all nested loops now
|
Thank you! |
return early when possible.
before this PR we built the whole result, merging part results and interpreting the combined overall result in the end.
we can save unnecessary work exiting early in case one of the part results are YES (similar how its done in
TrinaryLogic::maxMin).Wordpress repro
2.1.x
this PR
I think similar things can be done for union