BIND 10 #2206: define and implement ZoneTableSegment class

[BIND 10 Development]

#2206: define and implement ZoneTableSegment class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:7 jinmei]:
 > '''zone_table_segment.h'''
 >
 > - brief descriptions are mostly meaningless:
 > {{{#!cpp
 > /// \brief Zone Table Header Class
 > struct ZoneTableHeader {
 > }}}
 >   It's too obvious that this is a "Zone Table Header" class (or
 >   struct) from the type name.  I'd give some more informative one-line
 >   summary of what this class is.  I'd also provide some longer version
 >   of description to this particular class.  Some points also apply to
 >   other classes.

 Added.

 > - the return value description is mostly redundant:
 > {{{#!cpp
 >     /// Returns the MemorySegment used in this zone table segment.
 >     ///
 >     /// \return Returns a ZoneTableHeader for this zone table segment.
 > }}}
 >   I'd either provide more meaningful description or if there really
 >   isn't any just skipping the detailed version of the description.

 The redundant descriptions have been removed.

 > - `ZoneTableHeader`: maybe it's better to provide an accessor to
 >   table rather than having the caller be aware of offset_ptr (which is
 >   a kind of details that the caller doesn't have to care about):
 > {{{#!cpp
 > struct ZoneTableHeader {
 > public:
 >     ZoneTable* getTable() { return (table.get()); }
 >     // maybe we need a const version too?
 > private:
 >     boost::interprocess::offset_ptr<ZoneTable> table;
 > };
 > }}}

 Added.

 > - getHeader() should never return NULL.  I'd note that (or maybe we
 >   should return a reference instead of a pointer?  I'm not sure which
 >   one is better).

 I think the reference is better too, if it must point to something always.
 Updated in code.

 > - I'd note that the object returned by create() is dynamically
 >   allocated and the caller is responsible for deleting.  (providing a
 >   corresponding `destroy()` method maybe cleaner).

 Added.

 > - maybe a matter of taste in practice, I'd explicitly define the
 >   constructor of the base class as private to make it clear that the
 >   factory must be used to instantiate it.

 These have been made protected, and a friend as been added so that the
 factory method can instantiate it.

 > - not fully sure about this right now, but we probably want to have a
 >   const version of `getHeader()`.  The lookup code path should only
 >   need read-only access to the table.

 Added. Though not necessary, I've made the return values `const` in this
 case too.

 >
 > '''zone_table_segment_local'''
 >
 > - getHeader() and getMemorySegment(): for such very simple methods we
 >   might just define them in the header file.  It's purely a taste
 >   matter though, as they are virtual methods.

 I'll leave them in `zone_table_segment_local.cc` for now as that file is
 otherwise bare. I've added a comment to this file to move them once it has
 more method definitions.

 >
 > '''zone_table_segment_unittest'''
 >
 > - (general) scoped_ptr would be a slightly better choice than auto_ptr
 >   for such usage.

 This code is gone now as a fixture has been added.

 > - `config` is generated in every test.  You can avoid by introducing a
 >   fixture.  The total amount of code length wouldn't be much different
 >   though.

 A fixture has been added now.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2206#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development