Implement classes to successfully archive complicated object hierarchies

[yourpalal]

This ticket represents a part of my GSoC 2010 project, and relates directly to #5525. The classes contained in the patch allow for easy implementation of complicated archiving & unarchiving behaviours. Although these classes were created specifically to simplify archiving in the layout API, they should be useful elsewhere as well. These classes and the behaviours they expose have been tested fairly thoroughly.

[yourpalal]

forgot to set some things... :P

[yourpalal]

updated patch with fix for syntax error (oops, thanks luroh).

[bonefish]

Starting to review. Will probably take a few hours though and I don't know, if I'll have enough time to finish today. Will reply on the gsoc list. I don't want to discourage anyone else from reviewing, but please don't apply before I'm done.

[yourpalal]

Updated patch

[bonefish]

Applied in hrev37431.

[yourpalal]

Update to provide new template methods, better error handling, fix binary compatibility stuff, cleanup & refactor some code... probably more, also update Layout stuff to use the new methods provided & for a better archiving/unarchiving system.

[yourpalal]

slightly updated patch w/ better names, a little cleaner maybe, a specialization of BUnarchiver::InstantiateObject()

[bonefish]

Thanks! I applied the patch in hrev37538. I couldn't resist doing a few small modifications:

• In the BUnarchiver class definition I indented the enum ownership_policy type to the type column and inserted a blank line before the constructor declaration.
• I reorganized the generic BUnarchiver::InstantiateObject() implementation for two reasons:
  • While I preferred single-exit style function implementations (using cascaded checks or rechecks) myself several years ago (I don't recall whether I still used it when writing the layout code originally, but at least in the storage kit I did so consequently), I finally had to agree with others that the staggered-early-return style code is more readable, and to some degree even prevents programming mistakes (particularly when also using the RAII-style auto locker/deleter classes). So while not required by our coding style, I wholeheartedly recommend using it.
  • Somewhat related to the first point actually, the code relied on the method's BArchivable specialization to set the object return variable to NULL when returning an error (interim is deleted). While the specialized implementation does that indeed, from an API design point of view I find it only consequent that a failing function should not be required to do unnecessary things like setting reference parameters to NULL values. I don't really mind that the implementations do, but I don't think the API user should rely on this behavior.
• I changed the first if condition in BUnarchiveManager::GetArchivableForToken() to token >= fObjectCount.

[yourpalal]

Replying to bonefish:

Thanks! I applied the patch in hrev37538. I couldn't resist doing a few small modifications:

• In the BUnarchiver class definition I indented the enum ownership_policy type to the type column and inserted a blank line before the constructor declaration.
• I reorganized the generic BUnarchiver::InstantiateObject() implementation for two reasons:
  • While I preferred single-exit style function implementations (using cascaded checks or rechecks) myself several years ago (I don't recall whether I still used it when writing the layout code originally, but at least in the storage kit I did so consequently), I finally had to agree with others that the staggered-early-return style code is more readable, and to some degree even prevents programming mistakes (particularly when also using the RAII-style auto locker/deleter classes). So while not required by our coding style, I wholeheartedly recommend using it.

Sure, I will keep that in mind!

• Somewhat related to the first point actually, the code relied on the method's BArchivable specialization to set the object return variable to NULL when returning an error (interim is deleted). While the specialized implementation does that indeed, from an API design point of view I find it only consequent that a failing function should not be required to do unnecessary things like setting reference parameters to NULL values. I don't really mind that the implementations do, but I don't think the API user should rely on this behavior.

sure, that makes sense.

• I changed the first if condition in BUnarchiveManager::GetArchivableForToken() to token >= fObjectCount.

Thanks for the review & help!