search

ekinhbayar / gitamp

Listen to music generated by events across github.
MIT License
29 stars 10 forks source link

\micro-optimization #40

Closed pcrov closed 7 years ago

pcrov commented 7 years ago

For the greater glory of FQNs, of course.

mention-bot commented 7 years ago

@pcrov, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ekinhbayar and @PeeHaa to be potential reviewers.

PeeHaa commented 7 years ago

I am not sure I am going to be able to prefix all my global function calls with the global namespace identifier for such a tiny performance benefit.

Also for me personally it makes code worse to read. The alternative of "importing" individual functions using use is also kinda sucky.

As it currently is I am leaning towards rejecting this PR, however I want to keep it open for now so others (everybody but @Netmosfera ) can weigh in with arguments.

ekinhbayar commented 7 years ago

I truly dislike having the need to prefix all global function calls every single time. It would be very ugly and annoying to do so. But, if this is how PHP (as it is at the moment) can figure out and optimize them, then it feels like I should get into the habit of prefixing accordingly until there are better alternatives.

morrisonlevi commented 7 years ago

The alternative of "importing" individual functions using use is also kinda sucky.

I agree there.

I also don't like how PHP's behavior when looking up symbols that don't currently exist differs based on the symbol type. Unqualified functions get looked up in the global namespace while classes will trigger an autoload if necessary and then fail without looking it in the global namespace. I believe constants exhibit behavior that differs as well.

Those two factors coupled with the slight performance benefit is why I personally lean towards using the fully qualified function names in my own projects.

pcrov commented 7 years ago

Sorry, I should've provided an actual case for this instead of just a short quip. Basically my reasoning is what Levi said, with the addition that consistency and readability issues disappear with habit after only a very short time.

Wes0617 commented 7 years ago

Let's put it this way: phpstorm EAP (2017) behaves in a totally transparent way. Basically, you type in "strlen" and "use function strlen;" gets added at the top of the file's imports. There is no effective change to what you are used to do. You only get a slightly lengthier imports list (which you can keep collapsed though). What are your opinions now, given this feature?

Wes0617 commented 7 years ago

Also, if you don't like the big imports list, you can set PHPStorm to automatically replace to \strlen as soon you type strlen in...

Wes0617 commented 7 years ago

And also also... @PeeHaa: eat a bowl of dingleberries

ekinhbayar commented 7 years ago

you can set PHPStorm to automatically replace to \strlen as soon you type strlen in...

This is what I would prefer doing, instead of importing all the already built-in functions I need.

I wouldn't want to have any other lines of code to tell the language I will use the strlen and only then use it slightly more optimized. Specially thinking this for built-ins, which makes me wonder why they are firstly checked in the current ns anyway. I would expect it to be vice versa but in a sense I get it.

In any way it feels like I should be getting into the habit already, so I would merge the PR and keep prefixing, although it will be very annoying.

PeeHaa commented 7 years ago

I am going to merge this in and open a new ticket to set up some tooling around this.

I am still not 100% sure whether I can get used to this, but I am willing to give it a chance.

My current main concern about the maintenance from this point on is making sure all new commits use the fqn for calls and making it hard to not use fqn for new code.

PeeHaa commented 7 years ago

I would love to get some suggestions on this btw.