Filter get methods and properties #275
Conversation
| { | ||
| return array_values($this->getMethodsIndexedByName()); | ||
| if (null === $filter) { |
There was a problem hiding this comment.
@roukmoute no, please don't do that, unless you aren't using array_map() :-P
There was a problem hiding this comment.
@Ocramius But I've never understood why you prefer to test null while a function exists.
Can you clarify me on this point?
There was a problem hiding this comment.
@roukmoute is_null() is a function: while it allows for function composition, calling functions in PHP is the slowest thing ever, and the first/simplest thing you can do to remove O(1) overhead on any codebase.
To clarify:
// good
array_keys(array_filter([null, 1, 2, null], 'is_null'));
// bad
if (is_null($something)) {
// ...
}
// good
if (null === $something) {
// ...
}There was a problem hiding this comment.
Thanks @Ocramius for your great explanation :)
But I've few remarks:
is_null()does the job in C, and the comparison is not the same things, is it a mistake? In fact is it not too much to optimize this kind of things? (This is a real question 😄 )- You talking about big O notation, is it really necessary to think to this low level ? But I do a benchmark, and finally with PHP 7.1.4,
is_nullis more fastest. I don't know for previous versions.
Here is a small screencast about that.
There was a problem hiding this comment.
@roukmoute you forgot to namespace it :-)
There was a problem hiding this comment.
@Ocramius Can not agree with you that calling special functions are slow. Because you missing the presence of Optimizer in PHP. So if you will use \is_null($something); then it won't be function call at all, because PHP optimizer will inline it as simple type check.
To prove it, check https://3v4l.org/Ivbid/vld#output to see TYPE_CHECK opcode for that. And it's very fast 😄 But without leading slash it will be slow function call: https://3v4l.org/asV6T/vld#output
There was a problem hiding this comment.
Yes, well aware of the optimisation - still a huge load of noise for nothing IMO
src/Reflection/ReflectionClass.php
Outdated
| @@ -338,20 +338,33 @@ private function getMethodsIndexedByName() : array | |||
| /** | |||
| * Fetch an array of all methods for this class. | |||
| * | |||
| * @param ?int $filter | |||
There was a problem hiding this comment.
More documentation is needed on $filter, with examples, since this is a bitmask.
src/Reflection/ReflectionClass.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Get only the methods that this class implements (i.e. do not search | ||
| * up parent classes etc.) | ||
| * | ||
| * @param ?int $filter |
src/Reflection/ReflectionClass.php
Outdated
| @@ -478,40 +493,47 @@ public function getConstructor() : ReflectionMethod | |||
| * Get only the properties for this specific class (i.e. do not search | |||
| * up parent classes etc.) | |||
| * | |||
| * @param ?int $filter | |||
src/Reflection/ReflectionClass.php
Outdated
| return array_filter( | ||
| $this->cachedProperties, | ||
| function (ReflectionProperty $property) use ($filter) { | ||
| return null === $filter || $filter & $property->getModifiers(); |
There was a problem hiding this comment.
Weird that in this scenario you went for the "always true" condition in the filter callback
src/Reflection/ReflectionClass.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Get the properties for this class. | ||
| * | ||
| * @param ?int $filter |
src/Reflection/ReflectionObject.php
Outdated
| @@ -95,9 +95,10 @@ public static function createFromInstance($object) | |||
| /** | |||
| * Reflect on runtime properties for the current instance | |||
| * | |||
| * @param ?int $filter | |||
There was a problem hiding this comment.
same - maybe we should use @see and point to a single location
There was a problem hiding this comment.
would it be ok to point directly to http://php.net/manual/en/reflectionclass.getmethods.php where the parameter meaning is explained?
|
Well aware of that optimisation step, still a lot of noise though.
…On 17 May 2017 5:16 p.m., "Alexander Lisachenko" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Reflection/ReflectionClass.php
<#275 (comment)>
:
> {
- return array_values($this->getMethodsIndexedByName());
+ if (null === $filter) {
@Ocramius <https://github.com/ocramius> Can not agree with you that
calling special functions are slow. Because you missing the presence of
Optimizer in PHP. So if you will use \is_null($something); then it won't
be function call at all, because PHP optimizer will inline it as simple
type check.
To prove it, check https://3v4l.org/Ivbid/vld#output to see TYPE_CHECK
opcode for that. And it's very fast 😄 But without leading slash it will
be slow function call: https://3v4l.org/asV6T/vld#output
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#275 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakEvKw2OwuKItWV5YJqStEpqlekXlks5r6w8vgaJpZM4NdgaU>
.
|
|
@Ocramius I updated the PR with the fixes you suggested |
|
@marcosh awesome work! 👍 |
solves #103