mpqapi: saner handling of hash/block tables

[vlolteanu]

The rationale for this is two-fold:

• std::vector is used to store the tables; new and delete are gone
• Caching of the tables was brittle: basically, the programmer basically promised that the same archive would be loaded next time, and that was that. This PR checks the name of the file; if it doesn't match, the cache is disregarded.

[AJenbo]

Haven't looked at this, but i'm pretty excited :D

[ephphatha]

This always uses a static size? Can blockTbl be made a std::array instead of a vector?

[vlolteanu]

It can be made a std::array, but if you want to move the tables quickly to/from the cache (rather than copying them), you'd have to go the extra step and make it a std::unique_ptr<std::array>>. This would mess up the syntax when accessing the tables.

[glebm]

Rather than a vector for a fixed-size array, use a unique_ptr<T[]>. It is specialised for square brackets access.

[vlolteanu]

What would the advantage of using unique_ptr<T[]> over vector be? We lose the ability to do range-based for loops.

[glebm]

A vector is not the appropriate type for a fixed-size container. While it offers the nicest syntax among the options in stdlib, that alone not a good reason to use the wrong abstraction. In this case, unique_ptr<array> is also a good option.

[vlolteanu]

I really don't see how unique_ptr<T[]> is superior to vector in this case, when expressing and enforcing the container size is the main issue:

• vector is resizable when it ideally shouldn't be.
• unique_ptr<T[]> has indeterminate size, and can be replaced with an array of a different size.

[glebm]

Sounds like unique_ptr<array> is the most appropriate type here, despite somewhat awkward access syntax

[vlolteanu]

Making this a draft for the moment. I've got a few more commits coming.

[vlolteanu]

I've noticed problems w.r.t. error handling, or rather, lack thereof. Both mpqapi_write_file and mpqapi_flush_and_close can return false is case of error, but the result is always ignored. Both errors have something to do with writing stuff to the save file.

Should Diablo outright crash when something like this happens?

[StephenCWills]

For one thing, the FStreamWrapper class uses a CheckError() function to log the errors when they occur so they're not entirely ignored. I imagine that in the worst case, a failure to write could corrupt a save file. That's not something that would necessarily prevent the player from continuing their game so ignoring it isn't going to cause any immediate problems.

On the other hand, the game doesn't produce any log files so the log messages would get lost in most cases. I believe a failure to write is typically an indication of either a full disk, a permissions issue, or a hardware failure. Such errors should be rare, should typically manifest early on, and often require the user to take action.

I'd say crashing is probably better than doing nothing, but given how unlikely these errors probably are, I don't feel very strongly one way or the other. Just my two cents.

[vlolteanu]

Avoiding corrupt saved games would, in essence, require atomic modifications to the MPQs. It would probably be done as follows:

• Create a backup before the first modification of an MPQ, and call fsync (I'm not sure if there's a portable version). Crash if the backup can't be made. (Newly-created MPQs are exempt from this.)
• Before closing a modified MPQ, call fsync. Delete the backup if successful. Crash if the backup can't be deleted.
• When attempting to open an MPQ with an existing backup, attempt to restore the backup and call fsync. Delete the backup if successful, crash otherwise.