BIND 10 #517: Empty node processing in MemoryZone difficult Part

[BIND 10 Development]

#517: Empty node processing in MemoryZone difficult Part
-------------------------------------+-------------------------------------
                 Reporter:  hanfeng  |                Owner:  jinmei
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  A-Team-
                 Priority:  major    |  Sprint-20110209
                Component:  data     |           Resolution:
  source                             |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  0.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I've completed the review.  I have some design level comments (see below
 for details).

 '''Documentation in general'''

 Overall, documentation quality didn't seem sufficient to me:
  - In some cases existing review comments weren't addressed.
  - in some cases some out-of-context copy-past blocks were inserted,
    making the entire description unreadable.
  - existing documentation that needs to be updated with the changes
    of this ticket were not fully updated.
  - exception considerations were generally missing
  - param/return were still missing in some cases

 Rather than running further cycles of comment/revise/re-review,
 however, I've taken the liberty of updating the documentation by
 myself (not all of them, though), as you're taking off.  They've been
 committed and pushed to the repository.

 Next time, please make sure the documentation covers these basic
 points.

 '''Style issues in general'''

 newly committed code had a non negligible number of style
 violations.  I suspect the style you are familiar with is so
 different from our guideline that you cannot follow the guideline
 just by trying to be careful.  next time please use a tool (some
 script or a batch sequence of shell commands) to pre-detect common
 violations (in particular, s/foo */foo* /, s/bar &/bar& /, and
 s/return(x)/return (x)).

 '''successor()'''
  - it didn't fully address the review comment: It still didn't explain
    when to return NULL.  Exception consideration is also missing.
    I've updated the documentation including these points.

 '''NodeChain'''
  - it's now belong to the isc::datasrc namespace level, and the class
    name seems too generic.  I'd either rename it to something like
    RBTNodeChain or move it inside the RBTree class.
  - there are many style violations (directly fixed).  PLEASE introduce
    a tool to detect these and run it before asking for review.
  - this comment is not really correct:
 {{{
 /// RBNode did not have parent
 /// pointers in them (for memory usage reasons) so there was no way to
 find
 /// the path back to the root from any given node.
 }}}
    In fact, RBNode does have a parent pointer.
  - overall, documentation is way too insufficient.  you seem to have
    taken some doc from BIND 9 (which is itself not necessarily bad),
    so look at the BIND 9 doc again; it explains a lot of details of
    the chain.  We need the same level of documentation.
    methods need to be described.  (I've not touched the documentation
    of this class yet)
  - now that this is a separate class with non trivial code logic,
    it must have tests.  And, it should have been tested before.
    next time please make sure you write test first.
  - as for tests, we should especially cover invalid number of pops and
    pushes.  I'd also write a test where an RBTree contains a maximum
    number of levels and a chain has the maximum number of nodes.
  - I'd use exception instead of assert() due to bad inputs for various
    methods.  assert() is dangerous because careless use of library can
    crash the whole application.  Also, exceptions are more easily
    tested.
  - why RBT_MAX_LEVEL is an enum?  I'd use a static const.
  - what's the rationale of 254?  I know BIND 9 uses it, but I suspect
    it's unnecessarily big, and that's probably due to the leftover
    from the version in which bitstring labels were supported.  In any
    case, we should be able to explain why 254 is the maximum
    independently, and confirm it via a test.

 '''nextNode()'''
  - after re-reading it, I now have some concern about the  interface
    design: passing the node and chain is a bad idea, because it's not
    guaranteed the given chain is valid for the given node.  In BIND 9,
    a node chain contains the "current node" (which is equivalent to
    the node parameter of the nextNode()).  I guess we should do the
    same.
  - (the first point aside) I'd pass a reference to node instead of a
    pointer because the pointer should never be NULL in this case.
  - there's another design level issue.  It can return a NULL_NODE(),
    but the existence of such a node is hidden (which I think is rather
    a good thing though), and that would confuse the caller.  I don't
    know what is the best regarding this point, but maybe we should
    return a NULL pointer in case we'd otherwise return NULL_NODE.
  - It looks like this method can return an empty node regardless of
    how the RBTree is constructed.  Is that correct, and if so,
    intentional?  Should we rather skip empty nodes for this method by
    default, too?  In any case this point should be clearly documented
    and tested.

 '''tests'''

 I've made some editorial changes, and added one minor test.

 We need some more tests as pointed out before.

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