BIND 10 #2105: introduce node deleter of new RBTree

[BIND 10 Development]

#2105: introduce node deleter of new RBTree
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120807
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Jinmei

 Replying to [comment:7 jinmei]:
 > (Although I don't think I can take on full review) some things I've
 > noticed:
 > - it didn't compile for me because `DeleterType` doesn't have the
 >   constructor explicitly defined.  my compiler rejects a const instant
 >   of it without the default constructor:
 > {{{#!cpp
 > class DeleterType {
 > public:
 >     DeleterType() {}
 > ...
 > };
 > }}}

 This has been added now.

 > - I suspect we need to pass a `MemorySegment` to the deleter's
 >   operator() (or whatever actually deletes the data).  We'll use a
 >   segment when we actually use it for `RdataSet`.

 Reading both this and vorner's later reply to this comment, I think it's
 best we leave it to whoever uses `DomainTree` to change the argument list
 when it's actually used. There's no interface enforced for `DeleterType`
 anyway.

 > - I'm not sure if it's a good behavior to autoamtically delete
 >   existing data on setData().  I'm also not sure if we can pass NULL
 >   to the deleter.  please discuss this with the reviwer.

 When we call `setData()`, the ownership of the data passes to the tree (as
 we delete the data when we destroy the tree). So to keep it consistent, we
 have to destroy it if we replace the data with something else.

 I agree with vorner we should allow NULL and let the DeleterType handle it
 if necessary. I've added a comment for this. Maybe MemorySegment's doc can
 be updated too, to say that `MemorySegment::deallocate()` may be passed
 NULL. When the implementation uses `free()` or `delete`, it need not be
 checked.


 Hi vorner

 Replying to [comment:10 vorner]:
 > But, that being said, I think we need a way to take the data out somehow
 without destruction ‒ some kind of release method.

 This can be added trivially, but do we have a use-case when it has to be
 released (and not deleted during overwrite)?

 > Deleting of NULL is usually a nop (both with delete and free()), so I
 think we can take an consistent approach. But then, I think it should be
 documented that it is required of the deleter.

 A comment has been added to explain this.

 > Also, why is data_ set in the body of constructor, if it well could be
 done in the initializer list?
 > {{{#!c++
 >      flags_(FLAG_RED | FLAG_SUBTREE_ROOT),
 >      labels_capacity_(labels_capacity)
 >  {
 > +    data_ = NULL;
 >  }
 > }}}

 It has been moved to the initializer list now.

 >
 > And, the template parameter list ‒ I think we usually place a space
 after the comma there, so it should be `DomainTree<T, DT>`, not
 `DomainTree<T,DT>`.

 Done. :)

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