BIND 10 #2906: define and implement (generic) ZoneTable class

[BIND 10 Development]

#2906: define and implement (generic) ZoneTable class
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130514
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:8 muks]:

 > * I got the following compile error when building the branch:
 [...]
 > This could be worked around using the following patch (as
 > `ZoneTableAccessor` already derives from `boost::noncopyable`:
 > {{{
 > -class ZoneTableAccessorCache : boost::noncopyable, public
 ZoneTableAccessor {
 > +class ZoneTableAccessorCache : public ZoneTableAccessor {
 > }}}
 >
 > I went ahead and pushed this, as otherwise it generated an intermixed
 > diff with with some comment updates I made in
 > `ZoneTableAccessorCache`. :)

 Ah, okay.  And I think the workaround is reasonable.

 > * Just to clarify, we allow the use of `struct` (vs. `class` keyword)
 >   when there is code in the structure in our coding style, correct? It's
 >   a nice way to say `ZoneSpec` is fully public, but I want to know if we
 >   have any (unwritten) restrictions in our coding guidelines about it.

 I'm afraid I don't understand the question...what do you mean by "when
 there is code in the structure in our coding style"?  Regarding the
 use of `struct` (instead of `class`) itself, I don't think there's any
 written guideline for BIND 10.  So far, the choice is up to the
 developer.  Personally, I tend to use `struct` when I use it as a mere
 composition of members, not providing any behavior through non trivial
 methods (and the case of `ZoneSpec` should meet this personal
 convention).  It's probably better if we have some unified policy on
 in which case we use `struct` or `class` as it might possibly cause
 some confusion, so I'm not opposing to it.  So far, though, I don't
 have a feeling that this has been a serious issue.

 > * In the documentation for `ZoneTableIterator` class, in the following
 >   text, isn't it the case that they go through all the zones until
 >   `isLast()` returns true?

 Good catch, you're right.  I fixed it (and one other similar error).

 > Also, it should likely say that "... subsequent calls to \c next() go
 > through all the zones..." instead of `getCurrent()`.

 Right, I've fixed it too.

 > * I am not sure if `it_begin_` and `it_end_` should be members of the
 >   iterator. They waste space there as they are properties of the
 >   `ZoneTableAccessorCache` itself. Instead of checking `isLast()`, we
 >   could test the current iterator for equality against
 >   `ZoneTableAccessor::end()`. This would lend nicely to any future
 >   refinement of the iterator too. Then again, maybe it is not
 >   straightforward to implement this due to other constraints.

 On a closer look, I realized `it_begin_` is clearly unnecessary, so I
 removed it.  As for `ZoneTableAccessor::end()`, I suspect it's not
 easy, if not impossible, to cleanly implement it for database-based
 data sources.  As documented, this was the main reason why I didn't
 make the whole interface std::iterator compatible.  And, without
 `end()`, we'll need something like `it_end_` for the cache
 implementation or some other overhead either in speed or space anyway,
 so I think it makes to keep the current approach.  I also think the
 space overhead for `it_end_` isn't a big issue, because standard
 iterators should generally be small enough, we use
 `ZoneTableAccessor::Iterator` in the form of a pointer and won't use
 too many of them at the same time.

 > * I pushed some minor comment update commits to the branch. Please check
 >   them.

 These are generally good or okay (for some I wouldn't change them that
 way, but I don't object).

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