BIND 10 #2750: support DomainTree::remove()

[BIND 10 Development]

#2750: support DomainTree::remove()
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20131001
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  7             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Replying to [comment:19 vorner]:
 > Replying to [comment:17 muks]:
 > > > The name of methods and comment suggests it would set the pointers
 > > > mutually (child to parent and parent to child), but it is not
 > > > so. Should the comment be reformulated and/or renamed?
 > > > {{{#!c++
 > > >     /// \brief Helper method used in many places in code to set
 > > >     /// parent-child links.
 > > >     void setParentChild(DomainTreeNode<T>* oldnode,
 > > > }}}
 > >
 > > Re-reading the code, all I can come up with is `setParentsChild()`
 (with
 > > an extra `s`) which is not much of an improvement. Can you suggest a
 > > name?
 >
 > That `s` would indeed be an improvement. Or something like connectChild?

 I've changed it to `connectChild()` now.

 > > > This seems to be unconditional. Is that OK?
 > > > {{{#!c++
 > > >         other->setParentChild(this, other, root_ptr);
 > > > }}}
 > >
 > > I didn't understand this. What is wrong here?
 >
 > I'm not sure something is wrong. But this sets the parent's child
 > without any condition. Is it possible the lower might not be direct
 > child of root_ptr in the modified tree? If not, why?

 I'm sorry I'm still not following it. As `lower` takes the place of
 `this`, it connects the parent's corresponding child ptr (either
 `left_`, `right_` or `down_` as the case may be) that used to point to
 `this` to point to `lower`.

 Definitely `lower` may not be direct child of `root_ptr` (`root_ptr`
 points to the root of the whole forest). In `connectChild()`, if the
 initial `lower->getParent()` returns `NULL`, it means that the other
 node (`this` in `exchange()`) being exchanged with `lower` was the root
 node, and now, `lower` must become the new root node.

 > > > The same kind of double indirection is with the `find` methods.
 > >
 > > I'm not sure if we can avoid the `find()` changes as they are `public`
 > > methods. Or am I missing the point?
 >
 > The point is exactly the same as abstractAccessorImpl. I don't think
 > we need the `findImpl` wrappers, these can be skipped and the
 > resulting `tree->find` called directly. These are private.

 These have been updated.

 > > > I think this was copy-pasted so many times someone (not necessarily
 > > > you) lost the explicit root mentioned in the comment.
 > > > {{{#!c++
 > > > // The full absolute names of the nodes in the tree with the
 addition of
 > > > // the explicit root node.
 > > > const char* const domain_names[] = {
 > > >     "c", "b", "a", "x.d.e.f", "z.d.e.f", "g.h", "i.g.h",
 "o.w.y.d.e.f",
 > > >     "j.z.d.e.f", "p.w.y.d.e.f", "q.w.y.d.e.f", "k.g.h"
 > > > };
 > > > }}}
 > >
 > > From how I read it, the comment seems to say that these are the names
 in
 > > the tree, along with the "." node. If the "." node was already there,
 it
 > > wouldn't say that explicitly.
 >
 > But then, would it say /explicit/ root there? The root node is
 > implicit in this list.

 Do you see the difference between:

 {{{#!c++
 // The full absolute names of the nodes in the tree.
 const char* const domain_names[] = {
     ".", "c", "b", "a", "x.d.e.f", "z.d.e.f", "g.h", "i.g.h",
 "o.w.y.d.e.f",
     "j.z.d.e.f", "p.w.y.d.e.f", "q.w.y.d.e.f", "k.g.h"
 };
 }}}

 and:

 {{{#!c++
 // The full absolute names of the nodes in the tree with the addition of
 // the explicit root node.
 const char* const domain_names[] = {
     "c", "b", "a", "x.d.e.f", "z.d.e.f", "g.h", "i.g.h", "o.w.y.d.e.f",
     "j.z.d.e.f", "p.w.y.d.e.f", "q.w.y.d.e.f", "k.g.h"
 };
 }}}

 This is why I think nothing needs to be changed here.

 Replying to [comment:20 vorner]:
 > Ah, also this one. Would it make sense to switch the condition to
 > positive one, for readability?
 >
 > {{{#!c++
 >         if (!node->getRight()) {
 >             child = node->getLeft();
 >         } else {
 >             child = node->getRight();
 >         }
 > }}}

 Updated. :)

 I've checked that `clang++` on FreeBSD builder (that used to fail
 before) now passes. It also passes on my local workstation with
 `clang++` (with additional changes from #3172, but this is not required
 for our builders that use older versions of this compiler).

 I'll put #3172 to review individually, but if you want to check with
 `clang++` on your workstation, please merge `trac3172` and try it.

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