#8715 closed patch
Merge fsnode-gsoc into trunk
| Reported by: | SF/david_corrales | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
This patch incorporates all the FSNode changes in the gsoc-fsnode branch back into trunk.
Among the changes are the factories for filesystem objects, new functionality in the abstract filesystem class and fixes for TODO's using these new methods.
I personally tested this patch in Linux and Windows (MinGW). They both build without problems using r28462, but trunk should work as well.
P.D. I've been missing from the IRC channels lately because someone is blocking me any IRC connection =/ I talked to my ISP and they're not blocking any ports so it must be outside. I'm working on a solution.
Ticket imported from: #1768757. Ticket imported from: patches/820.
Attachments (3)
Change History (10)
by , 17 years ago
| Attachment: | gsoc-fsnode trunk.patch added |
|---|
comment:1 by , 17 years ago
| Owner: | set to |
|---|
comment:2 by , 17 years ago
Hi David,
some feedback on your patch:
* The "backend/factories" dir is badly named, IMO, as "factories" could be for anything, but you use it for *FS node* factories exclusively. However, instead of renaming this, I propose that you simply merge all of its contents into the existing "backends/fs" dir. So the files in "backends/factories/posix" would end up in "backends/fs/posix".
* Most (all?) of your new source files are missing the mandatory GPL license comment at the start. Should be easy enough to fix, though.
* fs-factory-maker.cpp: It unwise to have a big set of mutually exclusive (!) set of commands/#includes covered in blocks of "#ifdef/#endif". Rather, I'd prefer if this code used "#if/#elif/.../#endif".
* Same file: any good reason to introduce a whole FilesystemFactoryMaker class, just for a single static method in it? This just adds code and fake OO boiler plating, w/o giving any advantage (or does it?). Why not use use a global function, like in the good old C days? :-)
* common/fs.h: The doxygen comment for FilesystemNode::exists() is incorrect (started by /* not /**)
* Same file: could you please update & clarify some doxygen comments, please? You should go through each of them, and fix them. For example, some still refer to methods which no longer exist, like isValid(). Others are rather vague about semanting, e.g. exists() (what does "invalid node" mean, how does one check for one?); isReadable & isWritable (it should be a bit more specific, and point out what these mean for files vs. dirs); for lookupFile(), document whether the search is depth-first or breadth-first (in particular, for the variant searching in multiple dirs).
* dc-fs.cpp: You seem to have a copy&paste bug there, replacing refs to "ronin" by "POSIX": -/* - * Implementation of the ScummVM file system API based on ronin. +/** + * Implementation of the ScummVM file system API based on POSIX.
* gui/options.cpp: Small code formatting issue, there is a "{" on a separate line: + if(dir.isWritable()) + {
* common/fs.cpp: more code formatting isues in the lookupFile() method, operator< and possibly others
* Due to recent changes, I had to manually fixup agi/sound.cpp to compile with the patch (trivial changes, though, like name() -> getName()). Same for lure/detection.cpp (note that lure & drascula are only being compiled if you manually enable them when invoking configure).
by , 17 years ago
| Attachment: | gsoc-fsnode trunk (rev 1).patch added |
|---|
Revised GSoC-fsnode to trunk patch
by , 17 years ago
| Attachment: | gsoc-fsnode patch (sept 18).txt added |
|---|
FSNode patch updated for revision 28941.
comment:5 by , 17 years ago
| Status: | new → closed |
|---|
comment:7 by , 5 years ago
| Component: | → --Other-- |
|---|
gsoc-fsnode branch merge into trunk