Hey everyone, I created a patch some days ago to merge my work into trunk. I decided to take the conversation here to give it more exposure from other devs :)<br><br>The following is a reply by Max, which I posted here to further continue the discussion:
<br><br>>Hi David,<br>><br>>some feedback on your patch:<br>><br>>* The "backend/factories" dir is badly named, IMO, as "factories" could be<br>>for anything, but you use it for *FS node* factories exclusively. However,
<br>>instead of renaming this, I propose that you simply merge all of its<br>>contents into the existing "backends/fs" dir. So the files in<br>>"backends/factories/posix" would end up in "backends/fs/posix".
<br>><br>Actually, the factory files used to reside in the same directories as the FSNode implementations but I moved them out on purpose. I'm working on a BaseFile class which will use the same factories, so I decided that having a factory for file objects in the fsnode directory would be misleading. Also, if any other backend (say, the plugins one) switches to factory-based instantiation, the factories folder would be the ideal place to put a new kind of factory into.
<br><br>For me, it's a matter of clarity. You can check out my branch to see the new "file" folder and how the factories seem better in their own folder there.<br><br>In this regard, I'm waiting for your final word on where to put them :)
<br><br>>* Most (all?) of your new source files are missing the mandatory GPL<br>>license comment at the start. Should be easy enough to fix, though.<br>><br>Silly me, corrected!<br><br>>* fs-factory-maker.cpp
: It unwise to have a big set of mutually exclusive<br>>(!) set of commands/#includes covered in blocks of "#ifdef/#endif". Rather,<br>>I'd prefer if this code used "#if/#elif/.../#endif".<br>
><br>Already corrected this issue. The code does indeed look better.<br><br>>* Same file: any good reason to introduce a whole FilesystemFactoryMaker<br>>class, just for a single static method in it? This just adds code and fake
<br>>OO boiler plating, w/o giving any advantage (or does it?). Why not use use<br>>a global function, like in the good old C days? :-)<br>><br>This is another place where I chose a separate file for clarity. I can add the static method to the 
abstract-fs-factory.h file if you'd prefer it that way.<br><br>>* common/fs.h: The doxygen comment for FilesystemNode::exists() is<br>>incorrect (started by /* not /**)<br>><br>Weird, the comment reads "Indicates whether the object referred by this path exists in the filesystem or not." here, which sounds correct.
<br><br>>* Same file: could you please update & clarify some doxygen comments,<br>>please? You should go through each of them, and fix them. For example, some<br>>still refer to methods which no longer exist, like isValid(). Others are
<br>>rather vague about semanting, e.g. exists() (what does "invalid node" mean,<br>>how does one check for one?); isReadable & isWritable (it should be a bit<br>>more specific, and point out what these mean for files vs. dirs); for
<br>>lookupFile(), document whether the search is depth-first or breadth-first<br>>(in particular, for the variant searching in multiple dirs).<br>><br>I added more verbose comments to the methods you said and removed the references to isValid(). Additionally, I made the lookupFile() method breadth-first since I believe it fulfills the use case more properly.
<br><br>>* dc-fs.cpp: You seem to have a copy&paste bug there, replacing refs to<br>>"ronin" by "POSIX":<br>>-/*<br>>- * Implementation of the ScummVM file system API based on ronin.<br>
>+/**<br>>+ * Implementation of the ScummVM file system API based on POSIX.<br>><br>>* gui/options.cpp: Small code formatting issue, there is a "{" on a<br>>separate line:<br>>+                       if(
dir.isWritable())<br>>+                       {<br>><br>>* common/fs.cpp: more code formatting isues in the lookupFile() method,<br>>operator< and possibly others<br>><br>Fixed :D<br><br>><br>>* Due to recent changes, I had to manually fixup agi/sound.cpp to compile
<br>>with the patch (trivial changes, though, like name() -> getName()). Same<br>>for lure/detection.cpp (note that lure & drascula are only being compiled<br>>if you manually enable them when invoking configure).
<br>I did these changes on the patch too, at least the ones that compile by default. Is there a place I can check to see the ENV settings to build all possible cases (in order to correct any old method call).<br><br>I haven't yet committed the corrected patch since there's still things to discuss. As soon as they're cleared, I'll create the patch and publish it.
<br><br>