phpspec / prophecy

Highly opinionated mocking framework for PHP 5.3+
MIT License
8.53k stars 241 forks source link

Automated native_function_invocation fixes #449

Closed draco2003 closed 3 years ago

draco2003 commented 4 years ago

Reproduce with: php php-cs-fixer --rules=native_function_invocation fix ./ --allow-risky=yes (php-cs-fixer from : https://cs.symfony.com/)

Details: https://veewee.github.io/blog/optimizing-php-performance-by-fq-function-calls/

stof commented 4 years ago

There is a meaningful performance improvement only for compiler-optimized functions, as PHP will cache the resolution of function names anyway (we are talking about a 3ns difference between benchFqGlobalFunction and benchGlobalFunction in the benchmark, while the standard deviations of these runs are 18ns and 21ns respectively, so the difference is not statistically relevant) That's why the Symfony coding standards are to prefix the invocation only for compiler-optimized functions (php-cs-fixer maintains that list), as the extra \ everywhere was considered as being less readable.

I suggest doing the same here.

draco2003 commented 4 years ago

Agree that php-cs-fixer should be basing it off the list here: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php#L353 Will look into why it was more aggressively adding them.

Also other Libraries are opting for the use syntax instead: use function is_array;

If this improvement is desired, do you have a preference of one over the other?

stof commented 4 years ago

Well, if you run the fixer as --rules=native_function_invocation, it runs it with the default options, which are to fully-qualify all native functions. It don't think the CLI argument allows to specify options.

stof commented 4 years ago

use function is_array; is a no-go due to our supported PHP versions.