Merge branch 'work/rbtree/documentation'

src/lib/datasrc/memory_datasrc.cc

@@ -85,8 +85,8 @@ struct MemoryZone::MemoryZoneImpl {
         DomainNode* node;
         switch (domains->insert(name, &node)) {
             // Just check it returns reasonable results
-            case DomainTree::SUCCEED:
-            case DomainTree::ALREADYEXIST:
+            case DomainTree::SUCCESS:
+            case DomainTree::ALREADYEXISTS:
                 break;
             // Something odd got out
             default:

src/lib/datasrc/rbtree.h

@@ -17,11 +17,11 @@
 
 //! \file datasrc/rbtree.h
 ///
-/// \note The purpose of the RBTree is to provide a generic map with 
-/// domain names as the key that can be used by various BIND 10 modules or
-/// even by other applications.  However, because of some unresolved design
-/// issue, the design and interface are not fixed, and RBTree isn't ready to
-/// be used as a base data structure by other modules.
+/// \note The purpose of the RBTree is to provide a generic map with
+///     domain names as the key that can be used by various BIND 10 modules or
+///     even by other applications.  However, because of some unresolved design
+///     issue, the design and interface are not fixed, and RBTree isn't ready
+///     to be used as a base data structure by other modules.
 
 #include <dns/name.h>
 #include <boost/utility.hpp>
@@ -35,16 +35,18 @@ namespace datasrc {
 
 namespace helper {
 
-/// Helper function to remove the base domain from super domain
+/// \brief Helper function to remove the base domain from super domain.
 ///
-/// the precondition of this function is the super_name contains the
+/// The precondition of this function is the super_name contains the
 /// sub_name so
 /// \code Name a("a.b.c");
 /// Name b("b.c");
 /// Name c = a - b;
 /// \endcode
+/// c will contain "a".
 ///
-/// \note function in this namespace is not intended to be used outside.
+/// \note Functions in this namespace is not intended to be used outside of
+///     RBTree implementation.
 inline isc::dns::Name
 operator-(const isc::dns::Name& super_name, const isc::dns::Name& sub_name) {
     return (super_name.split(0, super_name.getLabelCount() -
@@ -55,63 +57,95 @@ operator-(const isc::dns::Name& super_name, const isc::dns::Name& sub_name) {
 template <typename T>
 class RBTree;
 
-/// \brief \c RBNode use by RBTree to store any data related to one domain name
+/// \brief \c RBNode is used by RBTree to store any data related to one domain
+///     name.
 ///
-/// It has two roles, the first one is as one node in the \c RBTree,
-/// the second one is to store the data related to one domain name and maintain
-/// the domain name hierarchy struct in one domain name space.
-/// As for the first role, it has left, right, parent and color members
-/// which is used to keep the balance of the \c RBTree.
-/// As for the second role, \c RBNode use down pointer to refer to all its sub
-/// domains, so the name of current node always relative to the up node. since
-/// we only has down pointer without up pointer, so we can only walk down from
-/// top domain to sub domain.
-/// One special kind of node is non-terminal node
-/// which has subdomains with RRset but itself doesn't have any RRsets.
+/// This is meant to be used only from RBTree. It is meaningless to inherit it
+/// or create instances of it from elsewhere. For that reason, the constructor
+/// is private.
 ///
-/// \note \c RBNode basically used internally by RBTree, it is meaningless to
-/// inherited from it or create it without \c RBTree.
+/// It serves three roles. One is to keep structure of the \c RBTree as a
+/// red-black tree. For that purpose, it has left, right and parent pointers
+/// and color member. These are private and accessed only from within the tree.
+///
+/// The second one is to store data for one domain name. The data related
+/// functions can be used to access and set the data.
+///
+/// The third role is to keep the hierarchy of domains. The down pointer points
+/// to a subtree of subdomains. Note that we can traverse the hierarchy down,
+/// but not up.
+///
+/// One special kind of node is non-terminal node. It has subdomains with
+/// RRsets, but doesn't have any RRsets itself.
 template <typename T>
 class RBNode : public boost::noncopyable {
-public:
-    /// only \c RBTree can create and destroy \c RBNode
+private:
+    /// The RBNode is meant for use from within RBTree, so it has access to
+    /// it.
     friend class RBTree<T>;
-    typedef boost::shared_ptr<T> NodeDataPtr;
 
-    /// \name Destructor
-    /// \note it's seems a little strange that constructor is private
-    /// but deconstructor left public, the reason is for some smart pointer
-    /// like std::auto_ptr, they needs to delete RBNode in sometimes, but
-    /// \code delete *pointer_to_node \endcode shouldn't be called directly
+    /// \name Constructors
+    ///
+    /// \note The existence of a RBNode without a RBTree is meaningless.
+    ///     Therefore the constructors are private.
     //@{
-    ~RBNode();
+
+    /// \brief Default constructor.
+    ///
+    /// This constructor is provided specifically for generating a special
+    /// "null" node.
+    RBNode();
+
+    /// \brief Constructor from the node name.
+    ///
+    /// \param name The *relative* domain name (if this will live inside
+    ///     a.b.c and is called d.e.a.b.c, then you pass d.e).
+    RBNode(const isc::dns::Name& name);
     //@}
 
-    /// \name Test functions
+public:
+    /// \brief Alias for shared pointer to the data.
+    typedef boost::shared_ptr<T> NodeDataPtr;
+
+    /// \brief Destructor
+    ///
+    /// It might seem strange that constructors are private and destructor
+    /// public, but this is needed because of shared pointers need access
+    /// to the destructor.
+    ///
+    /// You should never call anything like:
+    /// \code delete pointer_to_node; \endcode
+    /// The RBTree handles both creation and destructoion of nodes.
+    ~RBNode();
+
+    /// \name Getter functions.
     //@{
-    /// \brief return the name of current node, it's relative to its top node
+    /// \brief Return the name of current node.
+    ///
+    /// It's relative to its containing node.
     ///
     /// To get the absolute name of one node, the node path from the top node
-    /// to current node has to be recorded
+    /// to current node has to be recorded.
     const isc::dns::Name& getName() const { return (name_); }
 
-    /// \brief return the data store in this node
-    /// \note, since the data is managed by RBNode, developer should not
-    /// free the pointer
+    /// \brief Return the data stored in this node.
+    ///
+    /// You should not delete the data, it is handled by shared pointers.
     NodeDataPtr& getData() { return (data_); }
-    /// \brief return the data stored in this node, read-only version
+    /// \brief Return the data stored in this node.
     const NodeDataPtr& getData() const { return (data_); }
 
-    /// \brief return whether the node has related data
-    /// \note it's meaningless has empty \c RBNode in one RBTree, the only
-    /// exception is for non-terminal node which has sub domain nodes who
-    /// has data(rrset)
+    /// \brief return whether the node has related data.
+    ///
+    /// There can be empty nodes inside the RBTree. They are usually the
+    /// non-terminal domains, but it is possible (yet probably meaningless)
+    /// empty nodes anywhere.
     bool isEmpty() const { return (data_.get() == NULL); }
     //@}
 
-    /// \name Modify functions
+    /// \name Setter functions.
     //@{
-    /// \breif set the data stored in the node
+    /// \brief Set the data stored in the node.
     void setData(const NodeDataPtr& data) { data_ = data; }
     //@}
 
@@ -122,8 +156,6 @@ public:
     /// These methods never throw an exception.
     //@{
     /// Return if callback is enabled at the node.
-    ///
-    /// This method never throws an exception.
     bool isCallbackEnabled() const { return (callback_required_); }
 
     /// Enable callback at the node.
@@ -137,55 +169,45 @@ public:
 private:
     /// \brief Define rbnode color
     enum RBNodeColor {BLACK, RED};
-
-    /// \name Constructors
-    /// \note \c Single RBNode is meaningless without living inside one \c RBTree
-    /// the creation and destroy of one \c RBNode is handle by host \c RBTree, so
-    /// the constructors and destructor of \c RBNode is left private
-    //@{
-    /// \brief Default constructor.
-    ///
-    /// This constructor is provided specifically for generating a special
-    /// "null" node, and is intended be used only internally.
-    RBNode();
-
-    /// \brief Constructor from the node name.
-    ///
-    /// \param name The domain name corresponding to the node.
-    RBNode(const isc::dns::Name& name);
-    //@}
-
     /// This is a factory class method of a special singleton null node.
     static RBNode<T>* NULL_NODE() {
         static RBNode<T> null_node;
         return (&null_node);
     }
 
-    /// data to maintain the rbtree balance
+    /// \name Data to maintain the rbtree structure.
+    //@{
     RBNode<T>*  parent_;
     RBNode<T>*  left_;
     RBNode<T>*  right_;
     RBNodeColor color_;
+    //@}
 
+    /// \brief Relative name of the node.
     isc::dns::Name     name_;
+    /// \brief Data stored here.
     NodeDataPtr       data_;
 
-    /// the down pointer points to the root node of sub domains of current
-    /// domain
-    /// \par Adding down pointer to \c RBNode is for two purpose:
-    /// \li Accelerate the search process, with sub domain tree, it split the
-    /// big flat tree into several hierarchy trees
-    /// \li It save memory useage, so same label won't be saved several times
+    /// \brief The subdomain tree.
+    ///
+    /// This points to the root node of trees of subdomains of this domain.
+    ///
+    /// \par Adding down pointer to \c RBNode has two purposes:
+    /// \li Accelerate the search process, with sub domain tree, it splits the
+    ///     big flat tree into several hierarchy trees.
+    /// \li It saves memory useage as it allows storing only relative names,
+    ///     avoiding storage of the same domain labels multiple times.
     RBNode<T>*  down_;
 
-    // If true, callback should be called at this node in search.
-    // (This may have to become part of more general "attribute flags")
+    /// \brief If callback should be called when traversing this node in
+    /// RBTree::find().
+    ///
+    /// \todo It might be needed to put it into more general attributes field.
     bool callback_required_;
 };
 
 
-// typically each node should has a name associate with it
-// this construction is only used to create \c NULLNODE
+// This is only to support NULL nodes.
 template <typename T>
 RBNode<T>::RBNode() :
     parent_(this),
@@ -216,30 +238,39 @@ template <typename T>
 RBNode<T>::~RBNode() {
 }
 
-// note: the following class description is documented using C-style comments
+// note: the following class description is documented using multiline comments
 // because the verbatim diagram contain a backslash, which could be interpreted
-// as part of a multi-line comment with C++ style comments.
+// as escape of newline in singleline comment.
 /**
- *  \brief \c RBTree class represents all the domains with the same suffix,
- *  so it can be used to store the domains in one zone.
- * 
- *  \c RBTree is a generic red black tree, and contains all the nodes with
- *  the same suffix, since each name may have sub domain names
- *  so \c RBTree is a recursive data structure namely tree in tree.
- *  So for one zone, several RBTrees may be involved. But from outside, the sub
- *  tree is opaque for end users.
- * 
- *  \c RBTree split the domain space into hierarchy red black trees, nodes in one
- *  tree has the same base name. The benefit of this struct is that:
- *  - enhance the query performace compared with one big flat red black tree
- *  - decrase the memory footprint to save common labels only once.
- * 
+ *  \brief \c RBTree class represents all the domains with the same suffix.
+ *      It can be used to store the domains in one zone, for example.
+ *
+ *  RBTree is a generic map from domain names to any kind of data. Internally,
+ *  it uses a red-black tree. However, it isn't one tree containing everything.
+ *  Subdomains are trees, so this structure is recursive - trees inside trees.
+ *  But, from the interface point of view, it is opaque data structure.
+ *
+ *  \c RBTree splits the domain space into hierarchy red black trees; nodes
+ * in one tree has the same base name. The benefit of this struct is that:
+ *  - Enhances the query performace compared with one big flat red black tree.
+ *  - Decreases the memory footprint, as it doesn't store the suffix labels
+ *      multiple times.
+ *
+ * \anchor diagram
+ *
+ * with the following names:
+ *  - a
+ *  - b
+ *  - c
+ *  - x.d.e.f
+ *  - z.d.e.f
+ *  - g.h
+ *  - o.w.y.d.e.f
+ *  - p.w.y.d.e.f
+ *  - q.w.y.d.e.f
+ *
+ * the tree will looks like:
  *  \verbatim
-  with the following names:
-      a       x.d.e.f     o.w.y.d.e.f
-      b       z.d.e.f     p.w.y.d.e.f
-      c       g.h         q.w.y.d.e.f
-      the tree will looks like:
                                 b
                               /   \
                              a    d.e.f
@@ -253,33 +284,38 @@ RBNode<T>::~RBNode() {
                                      p
                                     / \
                                    o   q
- *  \endverbatim
+   \endverbatim
  *  \note open problems:
- *  - current find funciton only return non-empty nodes, so there is no difference
- *    between find one not exist name with empty non-terminal nodes, but in DNS query
- *    logic, they are different
+ *  - current \c find() function only returns non-empty nodes, so there is no
+ *    difference between find a non existent name and a name corresponding to
+ *    an empty non-terminal nodes, but in DNS query logic, they are different
  *  \todo
  *  - add remove interface
- *  - add iterator to iterate the whole rbtree while may needed by axfr
- *  - since \c RBNode only has down pointer without up pointer, the node path during finding
- *    should be recorded for later use
+ *  - add iterator to iterate over the whole \c RBTree.  This may be necessary,
+ *    for example, to support AXFR.
+ *  - since \c RBNode only has down pointer without up pointer, the node path
+ *    during finding should be recorded for later use
  */
 template <typename T>
 class RBTree : public boost::noncopyable {
     friend class RBNode<T>;
 public:
-    /// \brief The return value for the \c find() insert() and erase() method
+    /// \brief The return value for the \c find() and insert() methods
     enum Result {
-        SUCCEED, //insert or erase succeed
-        EXACTMATCH, //find the target name
-        PARTIALMATCH, //find part of target name
-        NOTFOUND,  // for find function means no related name found
-                   // for erase function means erase not exist name
-        ALREADYEXIST, //for insert operation, the name to insert already exist
+        SUCCESS,    ///< Insert was successful
+        /// \brief The node returned from find mathes exactly the name given
+        EXACTMATCH,
+        PARTIALMATCH, ///< A superdomain node was found
+        NOTFOUND,   ///< Not even any superdomain was found
+        /// \brief Returned by insert() if a node of the name already exists
+        ALREADYEXISTS,
     };
 
     /// \name Constructor and Destructor
     //@{
+    /// The constructor.
+    ///
+    /// It never throws an exception.
     explicit RBTree();
 
     /// \b Note: RBTree is not intended to be inherited so the destructor
@@ -287,130 +323,157 @@ public:
     ~RBTree();
     //@}
 
-    /// \name Inquery methods
-    //@{
-    /// \brief Find the node that gives a longest match against the given name
-    ///
-    /// This method searches the \c RBTree for a node whose name is a longest
-    /// match against \c name.  The found node, if any, is returned via the
-    /// \c node pointer.
-    /// By default, nodes that don't have data will be ignored, and the result
-    /// can be \c NOTFOUND even if there is a node whose name matches the
-    /// given \c name.
-    /// We'll soon introduce a "no data OK" mode in this method.  It would
-    /// match any node of the tree regardless of whether the node has data
-    /// or not.
-    /// Since the tree is "compressed", i.e., a node can contain multiple
-    /// name labels, there are counter intuitive cases in the "no data OK"
-    /// mode.  For example, see the diagram of the class description.
-    /// Name "y.d.e.f" is logically contained in the tree as part of the
-    /// "compressed" node of "w.y".  But the search logic of this method
-    /// cannot find the logical match, and would return a \c PARTIALMATCH
-    /// result pointing to node "d.e.f".  To correctly identify the real
-    /// longest match, "y.d.e.f" with empty data, the caller needs to
-    /// perform additional steps.
-    ///
-    /// This version of \c find() method is templated to allow the caller
-    /// to specify a "hook" at nodes that give a partial match.
-    /// When the search encounters a node with data that partially matches
-    /// \c name (i.e. node's name is a superdomain of \c name) and has
-    /// enabled callback (via the \c RBNode::enableCallback() method), if
-    /// \c callback is non \c NULL then the callback function is called
-    /// with the argument of a reference to the node and the given
-    /// callback argument (\c callback_arg).  The template parameter specifies
-    /// the type of the callback argument.
-    /// The callback function returns either \c true or \c false, meaning
-    /// the search should stop or continue, respectively.
-    /// If the return value is \c true the search stops immediately at the
-    /// node even if there could be a longer matching name below it.
-    /// In reality, this convoluted callback rule is specifically intended
-    /// to be used to handle a zone cut (delegation) at a name search inside
-    /// a zone, and won't be used in any other cases.
-    /// Other applications of the tree won't need callbacks, and they should
-    /// use the non templated version of the \c find() method.
-    ///
-    /// Since the expected usage of callback is very limited, we do not
-    /// generalize the interface so that it can be an arbitrary functions or
-    /// functor objects in favor of simplicity and efficiency.
-    ///
-    /// This method involves operations on names that can throw an exception.
+    /// \name Find methods
+    ///
+    /// \brief Find the node that gives a longest match against the given name.
+    ///
+    /// \anchor find
+    ///
+    /// These methods search the RBTree for a node whose name is a longest
+    /// against name. The found node, if any, is returned via the node pointer.
+    ///
+    /// By default, nodes that don't have data (see RBNode::isEmpty) are
+    /// ignored and the result can be NOTFOUND even if there's a node whose
+    /// name mathes. The plan is to introduce a "no data OK" mode for this
+    /// method, that would match any node of the tree regardless of wheather
+    /// the node has any data or not.
+    ///
+    /// The case with "no data OK" mode is not as easy as it seems. For example
+    /// in the diagram shown in the class description, the name y.d.e.f is
+    /// logically contained in the tree as part of the node w.y.  It cannot be
+    /// identified simply by checking whether existing nodes (such as
+    /// d.e.f or w.y) has data.
+    ///
+    /// These methods involves operations on names that can throw an exception.
     /// If that happens the exception will be propagated to the caller.
     /// The callback function should generally not throw an exception, but
     /// if it throws, the exception will be propagated to the caller.
     ///
+    /// The \c name parameter says what should be found. The node parameter
+    /// is output only and in case of EXACTMATCH and PARTIALMATCH, it is set
+    /// to a pointer to the found node.
+    ///
+    /// They return:
+    ///  - EXACTMATCH when a node with the same name as requested exists.
+    ///  - PARTIALMATCH when a node with the same name does not exist (or is
+    ///    empty), but there's a (nonempty) superdomain of the requested one.
+    ///    The superdomain with longest name is returned through the node
+    ///    parameter. Beware that if you store a zone in the tree, you may get
+    ///    PARTIALMATCH with zone apex when the given domain name is not there.
+    ///    You should not try to delegate into another zone in that case.
+    ///  - NOTFOUND if there's no node with the same name nor any superdomain
+    ///    of it. In that case, node parameter is left intact.
+    //@{
+
+    /// \brief Find with callback.
+    ///
+    /// \anchor callback
+    ///
+    /// This version of find calls the callback whenever traversing (on the
+    /// way from root down the tree) a marked node on the way down through the
+    /// domain namespace (see RBNode::enableCallback and related functions).
+    ///
+    /// If you return true from the callback, the search is stopped and a
+    /// PARTIALMATCH is returned with the given node. Note that this node
+    /// doesn't really need to be the one with longest possible match.
+    ///
+    /// This callback mechanism was designed with zone cut (delegation)
+    /// processing in mind. The marked nodes would be the ones at delegation
+    /// points. It is not expected that any other applications would need
+    /// callbacks; they should use the versions of find without callbacks.
+    /// The callbacks are not general functors for the same reason - we don't
+    /// expect it to be needed.
+    ///
     /// \param name Target to be found
     /// \param node On success (either \c EXACTMATCH or \c PARTIALMATCH)
-    /// it will store a pointer to the matching node
+    ///     it will store a pointer to the matching node
     /// \param callback If non \c NULL, a call back function to be called
-    /// at "delegation" nodes (see above).
+    ///     at marked nodes (see above).
     /// \param callback_arg A caller supplied argument to be passed to
-    /// \c callback.
-    ///
-    /// \return \c EXACTMATCH A node that whose name is equal to \c name is
-    /// found.  \c *node will be set to point to that node.
-    /// \return \c PARTIALMATCH There is a no exact match, but a superdomain
-    /// of \c name exists.  \c node will be set to point to the node whose
-    /// name is the longest among such superdomains.
-    /// \return \c NOTFOUND There is no exact or partial match against \c name
-    /// \c *node will be intact in this case.
+    ///     \c callback.
+    ///
+    /// \return As described above, but in case of callback returning true,
+    ///     it returns immediately with the current node.
     template <typename CBARG>
     Result find(const isc::dns::Name& name, RBNode<T>** node,
                 bool (*callback)(const RBNode<T>&, CBARG),
                 CBARG callback_arg) const;
 
-    /// Same as the other version, but the returned \c node will be immutable.
+    /// \brief Find with callback returning immutable node.
+    ///
+    /// It has the same behaviour as the find with \ref callback version.
     template <typename CBARG>
     Result find(const isc::dns::Name& name, const RBNode<T>** node,
                 bool (*callback)(const RBNode<T>&, CBARG),
                 CBARG callback_arg) const;
 
-    /// Same as the templated version, but does not use callback.
+    /// \brief Simple find.
     ///
-    /// Applications except the zone implementation should generally use the
-    /// non templated version.
+    /// Acts as described in the \ref find section.
     Result find(const isc::dns::Name& name, RBNode<T>** node) const {
         return (find<void*>(name, node, NULL, NULL));
     }
 
-    /// Same as the templated version, but does not use callback, and the
-    /// returned \c node will be immutable.
+    /// \brieg Simple find returning immutable node.
     ///
-    /// In general, this version should be preferred over the other non
-    /// templated version, unless the caller knows it should modify the
-    /// returned node.
+    /// Acts as described in the \ref find section, but returns immutable node
+    /// pointer.
     Result find(const isc::dns::Name& name, const RBNode<T>** node) const {
         return (find<void*>(name, node, NULL, NULL));
     }
-
-    /// \brief Get the total node count in the tree
-    /// the node count including the node created common suffix node,
-    /// this function will only be used for debuging
-    int getNodeCount() const { return (node_count_);}
     //@}
 
+    /// \brief Get the total number of nodes in the tree
+    ///
+    /// It includes nodes internally created as a result of adding a domain
+    /// name that is a subdomain of an existing node of the tree.
+    /// This function is mainly intended to be used for debugging.
+    int getNodeCount() const { return (node_count_); }
+
     /// \name Debug function
     //@{
-    /// \brief print the nodes in the trees
+    /// \brief Print the nodes in the trees.
+    ///
+    /// \param os A \c std::ostream object to which the tree is printed.
+    /// \param depth A factor of the initial indentation.  Each line
+    /// will begin with space character repeating <code>5 * depth</code>
+    /// times.
     void dumpTree(std::ostream& os, unsigned int depth = 0) const;
     //@}
 
-    /// \name Modify function
+    /// \name Modify functions
     //@{
-    /// \brief Insert the domain name into the tree
-    /// \param name The name to be inserted into the tree
-    /// \param inserted_node If no node with the name in the tree,
-    /// new \c RBNode will be created, otherwise nothing will be done.
-    /// Anyway the pointer point to the node with the name will be assigned to
-    /// inserted_node
+    /// \brief Insert the domain name into the tree.
+    ///
+    /// It either finds an already existing node of the given name or inserts
+    /// a new one, if none exists yet. In any case, the inserted_node parameter
+    /// is set to point to that node. You can fill data into it or modify it.
+    /// So, if you don't know if a node exists or not and you need to modify
+    /// it, just call insert and act by the result.
+    ///
+    /// Please note that the tree can add some empty nodes by itself, so don't
+    /// assume that if you didn't insert a node of that name it doesn't exist.
+    ///
+    /// This method normally involves resource allocation.  If it fails
+    /// the corresponding standard exception will be thrown.
+    ///
+    /// This method does not provide the strong exception guarantee in its
+    /// strict sense; if an exception is thrown in the middle of this
+    /// method, the internal structure may change.  However, it should
+    /// still retain the same property as a mapping container before this
+    /// method is called.  For example, the result of \c find() should be
+    /// the same.  This method provides the weak exception guarantee in its
+    /// normal sense.
+    ///
+    /// \param name The name to be inserted into the tree.
+    /// \param inserted_node This is an output parameter and is set to the
+    ///     node.
+    ///
     /// \return
-    //  - SUCCEED means no node exists in the tree with the name before insert
-    /// - ALREADYEXIST means already has the node with the given name
-    //
-    /// \node To modify the data related with one name but not sure the name has
-    /// inserted or not, it is better to call \c insert,instead of
-    /// \c find(), in case the name isn't exist and needs to insert again
+    ///  - SUCCESS The node was added.
+    ///  - ALREADYEXISTS There was already a node of that name, so it was not
+    ///     added.
     Result insert(const isc::dns::Name& name, RBNode<T>** inserted_node);
-    //@}
 
     /// \brief Swaps two tree's contents.
     ///
@@ -421,6 +484,7 @@ public:
         std::swap(NULLNODE, other.NULLNODE);
         std::swap(node_count_, other.node_count_);
     }
+    //@}
 
 private:
     /// \name RBTree balance functions
@@ -435,14 +499,15 @@ private:
     /// \brief delete tree whose root is equal to node
     void deleteHelper(RBNode<T> *node);
     /// \brief find the node with name
-    /// \param name is the target, up will points to the base domain of
-    /// the tree which name resides, node will point to the target node
-    /// if we has exact same name or partical name in current tree.
-    /// so for example, in zone a, we has
-    /// b.a, c.b.a and d.b.a search c.b.a, up will points to b.a.
-    /// and node will points to c.b.a
-    /// \note parameter up now is not used by any funciton, but we are gonna
-    /// need it soon to implement function like remove
+    ///
+    /// Internal searching function.
+    ///
+    /// \param name What should be found.
+    /// \param up It will point to the node whose down pointer points
+    ///     to the tree containing node. If we looked for o.w.y.d.e.f in the
+    ///     \ref diagram, the up would point to the w.y node.
+    ///     This parameter is not used currently, but it will be soon.
+    /// \param node The found node.
     template <typename CBARG>
     Result findHelper(const isc::dns::Name& name, const RBNode<T>** up,
                       RBNode<T>** node,
@@ -450,9 +515,7 @@ private:
                       CBARG callback_arg) const;
     void dumpTreeHelper(std::ostream& os, const RBNode<T>* node,
                         unsigned int depth) const;
-    /// for indent purpose, add certian mount empty charachter to output stream
-    /// according to the depth. This is a helper function which is only used when
-    /// dump tree
+    /// \brief Indentation helper function for dumpTree
     static void indent(std::ostream& os, unsigned int depth);
 
     /// Split one node into two nodes, keep the old node and create one new
@@ -611,7 +674,7 @@ RBTree<T>::insert(const isc::dns::Name& target_name, RBNode<T>** new_node) {
             if (new_node != NULL) {
                 *new_node = current;
             }
-            return (ALREADYEXIST);
+            return (ALREADYEXISTS);
         } else {
             const int common_label_count = compare_result.getCommonLabels();
             if (common_label_count == 1) {
@@ -664,7 +727,7 @@ RBTree<T>::insert(const isc::dns::Name& target_name, RBNode<T>** new_node) {
 
     ++node_count_;
     node.release();
-    return (SUCCEED);
+    return (SUCCESS);
 }
 
 template <typename T>
@@ -675,10 +738,13 @@ RBTree<T>::nodeFission(RBNode<T>& node, const isc::dns::Name& base_name) {
     // using auto_ptr here is to avoid memory leak in case of exception raised
     // after the RBNode creation
     std::auto_ptr<RBNode<T> > down_node(new RBNode<T>(sub_name));
+    node.name_ = base_name;
+    // the rest of this function should be exception free so that it keeps
+    // consistent behavior (i.e., a weak form of strong exception guarantee)
+    // even if code after the call to this function throws an exception.
     std::swap(node.data_, down_node->data_);
     std::swap(node.callback_required_, down_node->callback_required_);
     down_node->down_ = node.down_;
-    node.name_ = base_name;
     node.down_ = down_node.get();
     // root node of sub tree, the initial color is BLACK
     down_node->color_ = RBNode<T>::BLACK;

src/lib/datasrc/tests/rbtree_unittest.cc

@@ -90,62 +90,62 @@ TEST_F(RBTreeTest, setGetData) {
 }
 
 TEST_F(RBTreeTest, insertNames) {
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("d.e.f"), &rbtnode));
     EXPECT_EQ(Name("d.e.f"), rbtnode->getName());
     EXPECT_EQ(13, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("."), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("."), &rbtnode));
     EXPECT_EQ(Name("."), rbtnode->getName());
     EXPECT_EQ(14, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("example.com"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("example.com"), &rbtnode));
     EXPECT_EQ(15, rbtree.getNodeCount());
     rbtnode->setData(RBNode<int>::NodeDataPtr(new int(12)));
 
-    // return ALREADYEXIST, since node "example.com" already has been explicitly inserted
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("example.com"), &rbtnode));
+    // return ALREADYEXISTS, since node "example.com" already has been explicitly inserted
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("example.com"), &rbtnode));
     EXPECT_EQ(15, rbtree.getNodeCount());
 
     // split the node "d.e.f"
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("k.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("k.e.f"), &rbtnode));
     EXPECT_EQ(Name("k"), rbtnode->getName());
     EXPECT_EQ(17, rbtree.getNodeCount());
 
     // split the node "g.h"
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("h"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("h"), &rbtnode));
     EXPECT_EQ(Name("h"), rbtnode->getName());
     EXPECT_EQ(18, rbtree.getNodeCount());
 
     // add child domain
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("m.p.w.y.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("m.p.w.y.d.e.f"), &rbtnode));
     EXPECT_EQ(Name("m"), rbtnode->getName());
     EXPECT_EQ(19, rbtree.getNodeCount());
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("n.p.w.y.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("n.p.w.y.d.e.f"), &rbtnode));
     EXPECT_EQ(Name("n"), rbtnode->getName());
     EXPECT_EQ(20, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("l.a"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("l.a"), &rbtnode));
     EXPECT_EQ(Name("l"), rbtnode->getName());
     EXPECT_EQ(21, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("r.d.e.f"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("s.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("r.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("s.d.e.f"), &rbtnode));
     EXPECT_EQ(23, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("h.w.y.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("h.w.y.d.e.f"), &rbtnode));
 
     // add more nodes one by one to cover leftRotate and rightRotate
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("f"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("m"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("nm"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("om"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("k"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("l"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("fe"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("ge"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("i"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("ae"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("n"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("m"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("nm"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("om"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("k"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("l"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("fe"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("ge"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("i"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("ae"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("n"), &rbtnode));
 }
 
 TEST_F(RBTreeTest, findName) {
@@ -177,7 +177,7 @@ testCallback(const RBNode<int>&, bool* callack_checker) {
 
 TEST_F(RBTreeTest, callback) {
     // by default callback isn't enabled
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("callback.example"),
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("callback.example"),
                                                   &rbtnode));
     rbtnode->setData(RBNode<int>::NodeDataPtr(new int(1)));
     EXPECT_FALSE(rbtnode->isCallbackEnabled());
@@ -192,11 +192,11 @@ TEST_F(RBTreeTest, callback) {
     rbtnode->enableCallback();
     // add more levels below and above the callback node for partial match.
     RBNode<int>* subrbtnode;
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("sub.callback.example"),
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("sub.callback.example"),
                                                   &subrbtnode));
     subrbtnode->setData(RBNode<int>::NodeDataPtr(new int(2)));
     RBNode<int>* parentrbtnode;
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("example"),
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("example"),
                                                        &parentrbtnode));
     //  the chilld/parent nodes shouldn't "inherit" the callback flag.
     // "rbtnode" may be invalid due to the insertion, so we need to re-find

src/lib/datasrc/zonetable.cc

@@ -51,8 +51,8 @@ struct ZoneTable::ZoneTableImpl {
         ZoneNode* node(NULL);
         switch (zones_.insert(zone->getOrigin(), &node)) {
             // This is OK
-            case ZoneTree::SUCCEED:
-            case ZoneTree::ALREADYEXIST:
+            case ZoneTree::SUCCESS:
+            case ZoneTree::ALREADYEXISTS:
                 break;
             // Can Not Happen
             default: