Opened 12 years ago

Closed 12 years ago

Last modified 12 months ago

#8727 closed patch

lastPathComponent refactoring

Reported by: SF/david_corrales Owned by: SF/david_corrales
Priority: normal Component: --Other--
Keywords: Cc:
Game:

Description

As per Max's instructions I created this patch to address the lastPathComponent function in the FilesystemNodes.

I merged the code from said function into getName() since they both do the same thing. The hack in the Common::FS class is gone.

I also moved the matchString method to Common::Util and documented it with use cases and results.

Ticket imported from: #1804861. Ticket imported from: patches/832.

Attachments (3)

lastPathComponent (2).patch (39.4 KB ) - added by SF/david_corrales 12 years ago.
Patch to fix the lastPathComponent hack
getName (POSIX).patch (2.6 KB ) - added by SF/david_corrales 12 years ago.
Preview patch for the lazy instantiation
lastPathComponent (3).patch (20.0 KB ) - added by SF/david_corrales 12 years ago.
Static lastPathComponent patch

Download all attachments as: .zip

Change History (15)

by SF/david_corrales, 12 years ago

Attachment: lastPathComponent (2).patch added

Patch to fix the lastPathComponent hack

comment:1 by fingolfin, 12 years ago

This is much better already.

However, now getName() turned into an expensive operation for no good reason. When I said that "getName()" and "lastPathComponent" return the same value, I didn't mean that they should be merged.

Rather, it's fine for lastPathComponent to be an *internal* (static) function in the FS backends, which they call in e.g. their constructor to compute the value which later is returned by getName(). But getName() itself should be a "fast" operation, not one which walks through the whole path to compute the same constant value over and over again...

Another drawback of merging lastPathComponent into getName() itself is that now you have to call getName() in the constructor to compute the value of a member var. That's a potentially unsafe thing to do, as you are calling a public member method on a not fully initialised instance of your class, which could easily lead to errors.

So: You are on the right track, but leave lastPathComponent() as a static func, which is called from the constructors, and just change the code in FilesystemNode::lookupFileRec to use getName.

Also, I think there are too many examples in the Doxygen string of matchString. Shorten it to 2-5 examples or so. This should be in a separate patch anyway.

comment:2 by SF/david_corrales, 12 years ago

Ok, I already submitted a separate matchString patch with less comments :)

About the getName() method, why not use lazy instantiation? there's probably FilesystemNodes which don't even call the getDisplayName/getName method. Might save some clocks here or there and we avoid the static method.

comment:3 by fingolfin, 12 years ago

Saving a few clock cycles during instantiation time? I don't think that would be useful. Esp. if we need to complicate the code to achieve it.

Really, I think the code should be kept as simple as possible to ease maintanance -- it's already complicated enough now :/.

by SF/david_corrales, 12 years ago

Attachment: getName (POSIX).patch added

Preview patch for the lazy instantiation

comment:4 by SF/david_corrales, 12 years ago

We don't really need to complicate the code. In fact, we don't need lastPathComponent at all, just the getName function.
I posted a small sample (contains only the POSIX backend) with lazy instantiation so you can see what I mean.
File Added: getName (POSIX).patch

comment:5 by fingolfin, 12 years ago

No, I don't like to compute this "on the fly". The getName() method should return a fixed value for the FSnode, and not "change" over time. It's a bad hack to use a two-way execution path in it just to avoid a static function. I.e. one path is only used to init the object in the constructor, the other is used during the rest of the time. Except for coding mistakes, of course...

BTW, to check whether a string is empty, use the empty() method, it's more efficient than comparing to an empty string.

comment:6 by SF/david_corrales, 12 years ago

It wouldn' t actually be called on the constructor, but rather when needed. It's not really a hack, it's called lazy instantiation :) and it's a good practice when the operation costs a lot, so it's done until the very last possible moment.

Back on track. I assume that having getLastPathComponent as a static function is to avoid problems during object creation, because otherwise it'd be better to have it as a private member. I'll implement the patch this way and repost it.

comment:7 by fingolfin, 12 years ago

David, I *know* what lazy instantiation is, in fact I used that "design pattern" long before people started to talk about "design patterns" (as many good programmers did, of course). :-)

Still, in this case, it's just not appropriate. Extracting the last path component is not *that* costly. If it turns out to be a problem (e.g. when walking a file hierarchy), we can still speed it up by other means, but I wouldn't worry about that "problem" before we actualy *have* it. Anything else I'd call premature optimization ;-)

by SF/david_corrales, 12 years ago

Attachment: lastPathComponent (3).patch added

Static lastPathComponent patch

comment:8 by SF/david_corrales, 12 years ago

This is the patch using static lastPathComponent method in each backend. The only one I wasn't really sure about doing anything was the Playstation2 backend, so I left comments for the porter.

It's a lot smaller than the other patches since less changes are needed.
File Added: lastPathComponent (3).patch

comment:9 by fingolfin, 12 years ago

David, just saw your comment on the IRC logs. As a reminder (I said it before several times): If you want to talk to me -- just email me. Instead of asking people on IRC about me who won't know the answer anyway. :)

This patch seems fine and could be commited. By *you* :)

comment:10 by bluegr, 12 years ago

This has been commited by david_corrales with commit #29159. Is there any reason why this tracker item is still open?

comment:11 by SF/david_corrales, 12 years ago

Owner: set to SF/david_corrales
Status: newclosed

comment:12 by digitall, 12 months ago

Component: --Other--
Note: See TracTickets for help on using tickets.