[Scummvm-devel] Archive subclass inconsistencies (Was: [Scummvm-cvs-logs] SF.net SVN: scummvm:[52120] scummvm/trunk/common/fs.cpp (and r52121))

[Max Horn]

Am 17.08.2010 um 00:23 schrieb Max Horn:

> 
> Am 16.08.2010 um 23:52 schrieb Eugene Sandulenko:
>> 
>> 
>>> The directory entries are wrong and must go. What has to be debated
>>> now is which of these is supposed to be "correct": 1)
>>> m/foo/text.txt,  m/bar/text.txt, baz/1.dat, baz/2.dat 2)  text.txt,
>>> text.txt, 1.dat, 2.dat
>>> 
>>> In the original Archive design, the "flat" list in 2 would be the
>>> definite correct answer.
>> No, that's wrong. flat does not allow files with same name coexist in
>> different directories, and a warning is produced during load in such
>> cases. The reason is that the file name is used for hashmap key and
>> there is a clash occurs.
> 
> This is not a contradiction to what I wrote :). Yes, the internal hashmap of course can map every key (= file name) to only one value. And generates a warning. But in general, an Archive definitely *can* contain multiple members with the same name, you just cannot access them via getMember and hasFile. You still can access them by iterating over the result of listMembers, though. And it can't be different either (at least not with lots of extra work): Just think of SearchSet, which aggregates multiple heterogenous Archives together. It would have to perform expensive testing to detect inter-archive name clashes. Which we currently don't do, any I believe this is the right thing.


Actually, I have to correct myself: I wrote what I thought was true, but upon thinking about it again (followed by re-reading the code), I see that I was (half-)wrong: What I said is true for FSDir + FSNode, but not for most other Archive/Member implementations: For FSDir, archive members are FSNodes, and they implement a custom createReadStream() method that will just work, without any reference to the "parent" FSDir.

For most other Archive implementations, though, we rely on GenericArchiveMember, which does this:
  SeekableReadStream *GenericArchiveMember::createReadStream() const {
    return _parent->createReadStreamForMember(_name);
  }
Hence, if listMembers yields two entries with the same name, then both will open the same file, even though they ought to represent two different ones. And it gets worse once you start mixing different Archive subclasses via a SearchSet, as then behavior is totally erratic.

This is indeed quite a mess, partially caused by a lack of proper documentation and specification, and maybe also because we overlooked various issues at various stages. We should take a step back and redo quite some of this.

But first I need some sleep ;).


Bye,
Max