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

[BIND 10 Development]

#517: Empty node processing in MemoryZone difficult Part
-------------------------------------+-------------------------------------
                 Reporter:  hanfeng  |                Owner:  hanfeng
                     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 hanfeng):

 Replying to [comment:9 jinmei]:

 > '''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\
 RBTreeNodeChain is the new name for it and its interface is limited
 accessible
 by class except RBTree

 >    In fact, RBNode does have a parent pointer.
 parent pointer here means up pointer, I have changed it

 >  - overall, documentation is way too insufficient.  you seem to have
 >    taken some doc from BIND 9 (which is itself not necessarily bad),
 More document is added

 >  - 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.
 For the maximun is should be 128 which is decided by the MAX_LABEL in
 Name,
 since each node stores at least one label. For push, it's quite hard to
 test the exception, since Name has TooLongName check which stop me from
 create too deep tree.

 >  - 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.
 Exception is used instead of assert
 using enum is quite old style, since old version g++ doesn't support
 static const variable
 declare in class declare. I have modified it to const static int

 >
 > '''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
 >  - (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.
 RBTreeNodeChain is the only parameter for this function, and the only
 valid way to initialize it is through find function in RBTree

 >  - 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.
 NULLNODE is only used by RBTree inner logic, so returning NULL is the
 correct
 way, and for successor, since it is used by RBTree, so it's ok for it to
 return NULLNODE. nextNode is not like find, it's quite easy for user to
 skip empty
 node by keep invoking it. Comment has added for this behavior

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