Page MenuHomePhabricator

Use __METHOD__ / __FUNCTION__ instead of hardcoding function names in SemanticMediaWiki
Closed, ResolvedPublic

Description

Looking through a lot of the SMW code you're customly building function names

What's wrong with METHOD__/FUNCTION__

Simiarily, wfProfileIn( "SMWSQLStore2::getInProperties (SMW)" );

Do you need to add the "(SMW)"?? And why are you hard coding those anyway?


Version: unspecified
Severity: minor

Details

Reference
bz29432

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 21 2014, 11:36 PM
bzimport set Reference to bz29432.

And if the rest of the SM* extensions do the same, please clean those up aswell

I had a quick scan over the select() calls, and only found one not passing anything

Added in r90200

Can you evaluate any other DB methods that you're not passing them, and please add them too

The reason why "(SMW)" was added was that this simplifies getting an overview of all SMW-related code blocks among the large amount of profiling data. There does not seem to be another way of filtering profiling by extension, and this is important for a big extension like SMW.

In general, I am inclined to close this as invalid or at least wontfix. Clearly, we are discussing a detail of coding style here that should not have any impact on observable program behavior. Moreover, it is not so clear that the proposed change is an improvement at all. I the can see that the use of METHOD etc. could be considered preferable regarding code style, but OTOH the use of string construction functions in frequent profiling calls (to add "(SMW)") could be considered harmful for performance (since parameter construction costs apply even if profiling is disabled). I appreciate efforts to implement clean coding conventions, but looking at the code of SMW and MW, I wonder if this issue is anywhere near the top of things to bother about in this respect.

(This does not affect the unrelated comment on select; these should obviously have provenance information attached in all cases.)

(In reply to comment #3)

The reason why "(SMW)" was added was that this simplifies getting an overview
of all SMW-related code blocks among the large amount of profiling data. There
does not seem to be another way of filtering profiling by extension, and this
is important for a big extension like SMW.

If you want filterability, prefix with SMW- instead of suffixing with (SMW). It adds a nice expando-box on profileinfo.php. This will mess up sorting, though (they're all grouped together), so it may or may not actually be a good idea.

In general, I am inclined to close this as invalid or at least wontfix.
Clearly, we are discussing a detail of coding style here that should not have
any impact on observable program behavior.

I tend to agree.

Moreover, it is not so clear that
the proposed change is an improvement at all. I the can see that the use of
METHOD etc. could be considered preferable regarding code style, but OTOH
the use of string construction functions in frequent profiling calls (to add
"(SMW)") could be considered harmful for performance (since parameter
construction costs apply even if profiling is disabled).

That performance cost is minuscule and can be safely ignored. There's no need to prematurely optimise that.

Unknown Object (User) added a comment.Jun 8 2012, 11:52 PM

It is over a year that this request was discussed, can we close this one?