43
edits
Difference between revisions of "User talk:David corrales"
added Common::File discussion
m |
(added Common::File discussion) |
||
| Line 5: | Line 5: | ||
During the project development, the diagrams will become accurate and exact. | During the project development, the diagrams will become accurate and exact. | ||
== Current Filesystem API implementation == | == Common::File redesign discussion == | ||
While analizing the current state of the Common::File class, I've been noticing several important things that I feel, should be discussed. | |||
First of all, is the relevance of breaking up this class (like the FilesystemNode) one into a different implementation for each architecture. This is the primary motivation for this section. | |||
=== External references === | |||
The following table presents the files importing a given header throughout all of ScummVM: | |||
{| border="1" cellpadding="5" cellspacing="0" | |||
!Header File | |||
!Imports | |||
|- | |||
|file.h | |||
|105 | |||
|- | |||
|fs.h | |||
|32 | |||
|- | |||
|abstract-fs. | |||
|11 | |||
|} | |||
From this table it's easy to infer that the abstract-fs.h header has no issues. It's only called by the 10 backends plus its cpp file. | |||
The file.h header though, it's called from a -lot- of other files. This means that breaking the current Common::File implementation is like opening a pandora box. | |||
The fs.h doesn't seem as bad, although I haven't had the time to research it's use in each of the modules. Being a wrapper around the AbstractFilesystemNode, it's a lot easier to change the wrapped class, because the classes are designed to treat FilesystemNode as a wrapper. | |||
=== Inheritance === | |||
Another issue with breaking up Common::File is the Scumm::BaseScummFile class. If the Common::File gets broken in architecture specific implementations, creating an structure like this: | |||
{| border="1" cellpadding="5" cellspacing="0" | |||
!Level 0 | |||
!Level 1 | |||
!Level 2 | |||
|- | |||
|AbstractFile | |||
| | |||
| | |||
|- | |||
| | |||
|BaseScummFile | |||
| | |||
|- | |||
| | |||
| | |||
|ScummFile | |||
|- | |||
| | |||
| | |||
|ScummNESFile | |||
|- | |||
| | |||
|POSIXFile | |||
| | |||
|- | |||
| | |||
|WindowsFile | |||
| | |||
|- | |||
| | |||
|...File | |||
| | |||
|} | |||
Carefully looking, you'll see that BaseScummFile and all the architecture dependent implementations are on the same inheritance level! Of course this is not the intended behavior, since you'd want BaseScummFile to work with a backend like POSIXFile. In my second diagram I propose a solution to this issue. | |||
This solution requires the BaseScummFile to use a factory, in order to obtain the correct architecture dependent File implementation. The BaseScummFile would then defer invocations to said backend. | |||
Yet another, less than ideal, solution would be to use #ifdef to inherit the BaseScummFile from the correct backend. | |||
Finally, lets not forget that BaseScummFile uses other tricks to handle the inherited methods from Common::File. Since they are not virtual, the subclasses can create a method with the same signature and still call the base class independently (as done in ScummFile for example). By using the deferred method, this technique could pose problems. | |||
=== Methods affected by different architectures === | |||
The methods whose algorithm suffers changes depending on the current backend are only two: '''fopenNoCase''' and '''open.''' The '''fopenNoCase''' changes if the amigaos4, GP32 or palmos are defined. The '''open''' one changes only if MACOSX is defined. | |||
There's also methods whose signature gets redefined via the preprocessor using #define's. The architectures using this are three out of ten: Playstation 2, Dreamcast and Symbian32. This raises yet another problem. Since the algorithm doesn't change, to properly do this in subclasses, one would have to retype the whole algorithm -line by line- and only change '''fopen''' for say, '''ps2_fopen'''. Needless to say, this is not a good solution. | |||
Another option would be to use a pattern like, template for example. And define the algorithm in parts, where each subclass redefines parts as needed. This seems like overkill to me, since most of the backends would use the default implementation. As in a comment inside file.cpp, "such a high price for a simple beautification". | |||
=== A possible wrapper === | |||
Given that the Common::File class inherits itself from two classes: Common::SeekableReadStream and Common::WriteStream, the wrapper itself would have to handle things like "data >> wrapper", since streams usually overload the >> and << operators. Again, this would add a lot of complexity. | |||
=== New methods === | |||
Adding new methods to the existing Common::File class wouldn't have an impact, unless it redefines or changes the signature of the old ones. Instead of breaking up said class, it could be cleaned up and extended with new functionality. | |||
== Current Filesystem API implementation diagrams== | |||
[[Image:Fs1.png]] | [[Image:Fs1.png]] | ||
== Filesystem Node Redesign | == Filesystem Node Redesign proposal == | ||
[[Image:Fs2.png]] | [[Image:Fs2.png]] | ||
== Common::File Redesign | == Common::File Redesign proposal == | ||
[[Image:Fs3.png]] | [[Image:Fs3.png]] | ||
| Line 46: | Line 129: | ||
* an existing file (which hence might be writable or readable or both) | * an existing file (which hence might be writable or readable or both) | ||
* an existing directory (which can be writeable, too, with different meaning) | * an existing directory (which can be writeable, too, with different meaning) | ||
* a not yet existing but valid path to a file/dir (which is neither readable nor | * a not yet existing but valid path to a file/dir (which is neither readable nor writable, I guess). | ||
Well, that's not really a brilliant observation ; I just wanted to make sure this will be considered in the design process [[User:Fingolfin|Fingolfin]] 00:22, 24 March 2007 (CET) | Well, that's not really a brilliant observation ; I just wanted to make sure this will be considered in the design process [[User:Fingolfin|Fingolfin]] 00:22, 24 March 2007 (CET) | ||