Remove useless code #57868
Remove useless code #57868
Conversation
121fec1 to
296c8ee
Compare
...ymfony/Component/Form/Extension/Core/DataTransformer/IntegerToLocalizedStringTransformer.php
Outdated
Show resolved
Hide resolved
1b48b57 to
3258a9e
Compare
|
I'd say yes for a the following reasons:
Overall, it is a few optimization here and there and also a cleaner code with less "abandoned" method parameters and local variables that were maybe useful at some point but not anymore. |
3258a9e to
125f80c
Compare
There was a problem hiding this comment.
All those .= or += are perf optimizations. Not using them means creating new values in memory instead of mutating existing ones in place.
You can try the following script to confirm (replace the call to f2 by f1 and vice-versa):
function f1 () {
$a = str_repeat('-', 100000000);
return $a . $a;
}
function f2 () {
$a = str_repeat('-', 100000000);
return $a .= $a;
}
echo memory_get_peak_usage();
echo "\n";
f2();
echo memory_get_peak_usage();
echo "\n";
src/Symfony/Component/Serializer/Mapping/Loader/AttributeLoader.php
Outdated
Show resolved
Hide resolved
bb9e2c7 to
0a2f748
Compare
|
|
||
| return $str; |
There was a problem hiding this comment.
Oh alright, I missed the exact point of "All those .= or += are perf optimizations.". Should be good now, I also reverted all casters
There was a problem hiding this comment.
It's very debatable if we need such a perf optimization in a test class in my opinion. There are thousands of concatenating operators in the Symfony code base that don't do it in-place.
To be honest, all of the ones you commented on do not look like to me they are on the critical path of performance: it's either dev tools (browserkit, vardumper) or actions that are not in a critical path (the impersonator url generator might be the only one that we can argue might be called on every request).
Do we really need to keep all of them for added cognitive complexity?
There was a problem hiding this comment.
CS shouldn't be a reason to degrade perf (var-dumper is perf-critical, everybody wants fast dumps). That's also not worth the future merge conflicts. I don't see any upsides that outweighs the downsides.
src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php
Outdated
Show resolved
Hide resolved
0a2f748 to
58941ce
Compare
|
Thank you @alexandre-daubois. |
I went through the remaining components and bundles, following my last PRs of this kind.