Use TypeUtils::getOldConstantArrays in array_pop and array_shift extensions#1668
Conversation
|
I have a refactor idea and will try to get rid of the reference |
| return $this->valueTypes; | ||
| } | ||
|
|
||
| public function getFirstValueType(): Type |
There was a problem hiding this comment.
there's a getValueTypes right above which simply returns all valueTypes while this one is "smart" and returns the type that you would expect when you access the first or last element in PHP => it deals with optional keys correctly.
I was just afraid that naming-wise one might expect that this should simply return $this->valueTypes[0]. is the name OK, or does maybe getFirstValue or getFirstEntry or so make more sense? hmm
There was a problem hiding this comment.
in case the method is not re-usable/usefull as is for other use-cases you might move them as a private method into the extensions instead. that way its not public api and doesn't need in detail perfection.
There was a problem hiding this comment.
also possible, I agree, I'm fine with anything here I guess
|
was able to simplify this by not forcing re-usage of existing code. that existing code would have been slower anyways and do things that are not needed here |
do you have a benchmark somewhere? |
nope, I'd also like to avoid doing one just for this tbh. In general using I'm just going with what Ondrej also was recently doing basically where he used But you're right, it improves correctness, so the potential performance improvements are coming with it for free :) |
|
is this PR also expected to fix phpstan/phpstan#7804 ? |
I would not expect that, no. Here only ConstantArray behaviour and the return type handling are adapted. |
|
Nice! Thank you. |
3 participants
Since the logic for removing first and last elements was already there I wanted to re-use it. I'm not the biggest fan of passing this info by reference, but did not come up with something better yet.I'd like to get rid of
TypeUtils::getConstantArraysfor performance reasons where possible.