Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6256 closed enhancement (fixed)

Implement classes to successfully archive complicated object hierarchies

Reported by: Alex Owned by: axeld
Priority: normal Milestone: R1
Component: Kits/Support Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking: #5525, #6257, #6314
Has a Patch: yes Platform: All

Description

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.

Attachments (4)

archivable.patch (23.2 KB) - added by Alex 7 years ago.
updated patch with fix for syntax error (oops, thanks luroh).
archiving.patch (25.4 KB) - added by Alex 7 years ago.
Updated patch
archiving.2.patch (26.0 KB) - added by Alex 7 years ago.
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.
archiving.3.patch (26.2 KB) - added by Alex 7 years ago.
slightly updated patch w/ better names, a little cleaner maybe, a specialization of BUnarchiver::InstantiateObject()

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by Alex

Has a Patch: set

comment:3 Changed 7 years ago by Alex

Component: - GeneralKits/Support Kit
Owner: changed from Nobody to axeld
Type: bugenhancement
Version: R1/alpha2R1/Development

forgot to set some things... :P

Changed 7 years ago by Alex

Attachment: archivable.patch added

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

comment:4 Changed 7 years ago by Alex

Blocking: 6257 added

comment:5 Changed 7 years ago by Ingo Weinhold

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.

Changed 7 years ago by Alex

Attachment: archiving.patch added

Updated patch

comment:6 Changed 7 years ago by Ingo Weinhold

Resolution: fixed
Status: newclosed

Applied in hrev37431.

Changed 7 years ago by Alex

Attachment: archiving.2.patch added

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.

comment:7 Changed 7 years ago by Alex

Resolution: fixed
Status: closedreopened

comment:8 Changed 7 years ago by Alex

Blocking: 6314 added

Changed 7 years ago by Alex

Attachment: archiving.3.patch added

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

comment:9 Changed 7 years ago by Ingo Weinhold

Resolution: fixed
Status: reopenedclosed

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.

comment:10 in reply to:  9 Changed 7 years ago by Alex

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!

Note: See TracTickets for help on using tickets.