#8727 closed patch
lastPathComponent refactoring
| Reported by: | SF/david_corrales | Owned by: | SF/david_corrales |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | 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)
Change History (15)
by , 17 years ago
| Attachment: | lastPathComponent (2).patch added |
|---|
comment:1 by , 17 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 , 17 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 , 17 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 :/.
comment:4 by , 17 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 , 17 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 , 17 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 , 17 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 ;-)
comment:8 by , 17 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 , 17 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 , 17 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 , 17 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:12 by , 6 years ago
| Component: | → --Other-- |
|---|
Patch to fix the lastPathComponent hack