[Scummvm-devel] GSoC patch for trunk

[Max Horn]

Hi,
sorry for the late reply, but I had no chance to get near a compute  
during the past couple days.

Am 13.08.2007 um 21:09 schrieb David Corrales:

> 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 :)
>
> The following is a reply by Max, which I posted here to further  
> continue the discussion:
>
> >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".
> >
> 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.
>
> 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.
>
> In this regard, I'm waiting for your final word on where to put  
> them :)

It makes no sense to me to have a "factory" backend module. Factories  
are a tool in OO, a concept, a design pattern, but they are not  
something that should be treated as a generic entity by themselves.  
So, please, remerge them.


> >* 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? :-)
> >
> 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.

Yes, please, just turn this class into a simple global function.

Also, I agree with Ender: Splitting stuff into too many small files  
does not really help anybody, but can actually make it harder to  
navigate the codde.


>
>
> >* common/fs.h: The doxygen comment for FilesystemNode::exists() is
> >incorrect (started by /* not /**)
> >
> Weird, the comment reads "Indicates whether the object referred by  
> this path exists in the filesystem or not." here, which sounds  
> correct.

The content is fine, the formatting is not: A doxygen comment must  
start with /**, that comment does however start with /*.


>
> >
> >* 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).
> 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).

ENV setting? There is no such thing, but you can pass various -- 
enable-FOO arguments to configure -- see "configure --help". Right  
now I always use this (and I have a small shell script reconf.sh  
containing that line):

./configure --enable-Werror --enable-lure --enable-cruise --enable- 
drascula


>
>
> 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.

Please do so :)

Cheers,
Max