[svn] commit: r1886 - in /branches/trac49/src/lib/dns: name.cc name.h tests/name_unittest.cc
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu May 20 23:38:04 UTC 2010
Author: jinmei
Date: Thu May 20 23:38:04 2010
New Revision: 1886
Log:
changed createAncestorNameOf() to Name::split(level) and removed createSuffixNameOf(), based on this morning's call.
updated the document to reflect the design choice.
Modified:
branches/trac49/src/lib/dns/name.cc
branches/trac49/src/lib/dns/name.h
branches/trac49/src/lib/dns/tests/name_unittest.cc
Modified: branches/trac49/src/lib/dns/name.cc
==============================================================================
--- branches/trac49/src/lib/dns/name.cc (original)
+++ branches/trac49/src/lib/dns/name.cc Thu May 20 23:38:04 2010
@@ -666,7 +666,7 @@
}
Name
-Name::split(unsigned int first, unsigned int n) const {
+Name::split(const unsigned int first, const unsigned int n) const {
if (n == 0 || n > labelcount_ || first > labelcount_ - n) {
isc_throw(OutOfRange, "Name::split: invalid split range");
}
@@ -700,6 +700,16 @@
assert(retname.labelcount_ == newlabels);
return (retname);
+}
+
+Name
+Name::split(const unsigned level) const {
+ if (level >= getLabelCount()) {
+ isc_throw(OutOfRange, "invalid level for name split (" << level
+ << ") for name " << *this);
+ }
+
+ return (split(level, getLabelCount() - level));
}
Name&
Modified: branches/trac49/src/lib/dns/name.h
==============================================================================
--- branches/trac49/src/lib/dns/name.h (original)
+++ branches/trac49/src/lib/dns/name.h Thu May 20 23:38:04 2010
@@ -502,6 +502,72 @@
/// labels including and following the <code>first</code> label.
Name split(unsigned int first, unsigned int n) const;
+ /// \brief Extract a specified super domain name of Name.
+ ///
+ /// This function constructs a new \c Name object that is a super domain
+ /// of \c this name.
+ /// The new name is \c level labels upper than \c this name.
+ /// For example, when \c name is www.example.com,
+ /// <code>name.split(1)</code> will return a \c Name object for example.com.
+ /// \c level can be 0, in which case this method returns a copy of
+ /// \c this name.
+ /// The possible maximum value for \c level is
+ /// <code>this->getLabelCount()-1</code>, in which case this method
+ /// returns a root name.
+ ///
+ /// One common expected usage of this method is to iterate over super
+ /// domains of a given name, label by label, as shown in the following
+ /// sample code:
+ /// \code // if name is www.example.com...
+ /// for (int i = 0; i < name.getLabelCount(); ++i) {
+ /// Name upper_name(name.split(i));
+ /// // upper_name'll be www.example.com., example.com., com., and then .
+ /// }
+ /// \endcode
+ ///
+ /// \c level must be smaller than the number of labels of \c this name;
+ /// otherwise an exception of class \c OutOfRange will be thrown.
+ /// In addition, if resource allocation for the new name fails, a
+ /// corresponding standard exception will be thrown.
+ ///
+ /// Note to developers: probably as easily imagined, this method is a
+ /// simple wrapper to one usage of the other
+ /// <code>split(unsigned int, unsigned int) const</code> method and is
+ /// redundant in some sense.
+ /// We provide the "redundant" method for convenience, however, because
+ /// the expected usage shown above seems to be common, and the parameters
+ /// to the other \c split(unsigned int, unsigned int) const to implement
+ /// it may not be very intuitive.
+ ///
+ /// We are also aware that it is generally discouraged to add a public
+ /// member function that could be implemented using other member functions.
+ /// We considered making it a non member function, but we could not come
+ /// up with an intuitive function name to represent the specific service.
+ /// Some other BIND 10 developers argued, probably partly because of the
+ /// counter intuitive function name, a different signature of \c split
+ /// would be better to improve code readability.
+ /// While that may be a matter of personal preference, we accepted the
+ /// argument. One major goal of public APIs like this is wider acceptance
+ /// from internal/external developers, so unless there is a clear advantage
+ /// it would be better to respect the preference of the API users.
+ ///
+ /// Since this method doesn't have to be a member function in other way,
+ /// it is intentionally implemented only using public interfaces of the
+ /// \c Name class; it doesn't refer to private members of the class even if
+ /// it could.
+ /// This way we hope we can avoid damaging the class encapsulation,
+ /// which is a major drawback of public member functions.
+ /// As such if and when this "method" has to be extended, it should be
+ /// implemented without the privilege of being a member function unless
+ /// there is a very strong reason to do so. In particular a minor
+ /// performance advantage shouldn't justify that approach.
+ ///
+ /// \param level The number of labels to be removed from \c this name to
+ /// create the super domain name.
+ /// (0 <= \c level < <code>this->getLabelCount()</code>)
+ /// \return A new \c Name object to be created.
+ Name split(unsigned int level) const;
+
/// \brief Reverse the labels of a name
///
/// This method reverses the labels of a name. For example, if
@@ -613,63 +679,6 @@
/// \name Utility functions manipulating names.
///
//@{
-/// \brief Create an ancestor name of a given \c Name.
-///
-/// This function creates a new \c Name object that is an ancestor of
-/// \c source. The new name is \c level labels upper than \c source.
-/// For example, when \c source is www.example.com,
-/// <code>createAncestorNameOf(source, 1)</code> will return a \c Name object
-/// for example.com.
-/// \c level can be 0, in which case this function returns a copy of the
-/// \c source name.
-/// The possible maximum value for \c level is
-/// <code>source.getLabelCount()-1</code>, in which case this function
-/// returns a root name.
-///
-/// One common expected usage of this function is to iterate over super
-/// domains of a given name, label by label, as shown in the following
-/// sample code:
-/// \code // if name is www.example.com...
-/// for (int i = 0; i < name.getLabelCount(); ++i) {
-/// Name upper_name(createAncestorNameOf(name, i));
-/// // upper_name will be www.example.com., example.com., com., and then .
-/// }
-/// \endcode
-///
-/// \c level must be smaller than the number of labels of \c source;
-/// otherwise an exception of class \c OutOfRange will be thrown.
-/// In addition, if resource allocation for the new name fails, a
-/// corresponding standard exception will be thrown.
-///
-/// Note to developers: probably as easily imagined, this function is a
-/// simple wrapper to one usage of the \c Name::split() method and is
-/// redundant in some sense.
-/// We provide this as a public function for convenience, however, because
-/// the expected usage shown above seems to be common, and the parameters
-/// to \c Name::split() to implement it may not be very intuitive.
-/// Another possible question about this function might be why this is not
-/// implemented as a public member function of the \c Name class.
-/// This is because we wanted to keep the class definition as simple as
-/// possible by limiting public members to something primitive to the class
-/// (being a wrapper of another member, this function is clearly not
-/// primitive).
-/// And, in fact, this is generally considered a better practice in C++;
-/// if we implemented this function as a member function, it might exploit
-/// its ability to access private class members for, e.g., performance
-/// reasons, which would make it harder to change the internal implementation
-/// of the class in future.
-/// One possible disadvantage of non member function approach is that
-/// it might be less efficient than a member function providing the same
-/// service. At the moment we have no evidence that the performance overhead
-/// is a real issue, so we follow the common practice of the language.
-/// If, through performance benchmark in future, it turns out to be a
-/// significant bottleneck in critical operations, we may change the decision
-/// and may consider implementing it as a member function of the \c Name class.
-///
-/// \param source A \c Name for which the ancestor name is to be created.
-/// \param level The number of labels to be removed from \c source to create
-/// the ancestor name. (0 <= \c level < <code>source.getLabelCount()</code>)
-/// \return A new \c Name object to be created.
Name
createAncestorNameOf(const Name& source, unsigned int level);
Modified: branches/trac49/src/lib/dns/tests/name_unittest.cc
==============================================================================
--- branches/trac49/src/lib/dns/tests/name_unittest.cc (original)
+++ branches/trac49/src/lib/dns/tests/name_unittest.cc Thu May 20 23:38:04 2010
@@ -500,25 +500,16 @@
OutOfRange);
}
-TEST_F(NameTest, createAncestorNameOf) {
- EXPECT_EQ(Name("example.com"), createAncestorNameOf(example_name, 1));
- EXPECT_EQ(example_name, createAncestorNameOf(example_name, 0));
- EXPECT_EQ(Name("."), createAncestorNameOf(example_name, 3));
+TEST_F(NameTest, split_for_suffix) {
+ EXPECT_PRED_FORMAT2(UnitTestUtil::matchName, example_name.split(1),
+ Name("example.com"));
+ EXPECT_PRED_FORMAT2(UnitTestUtil::matchName, example_name.split(0),
+ example_name);
+ EXPECT_PRED_FORMAT2(UnitTestUtil::matchName, example_name.split(3),
+ Name("."));
// Invalid case: the level must be less than the original label count.
- EXPECT_THROW(createAncestorNameOf(example_name, 4), OutOfRange);
-}
-
-TEST_F(NameTest, createSuffixNameOf) {
- EXPECT_EQ(Name("."), createSuffixNameOf(example_name, 1));
- EXPECT_EQ(Name("example.com"), createSuffixNameOf(example_name, 3));
- EXPECT_EQ(example_name, createSuffixNameOf(example_name,
- example_name.getLabelCount()));
- // Invalid cases
- EXPECT_THROW(createSuffixNameOf(example_name, 0), OutOfRange);
- EXPECT_THROW(createSuffixNameOf(example_name,
- example_name.getLabelCount() + 1),
- OutOfRange);
+ EXPECT_THROW(example_name.split(4), OutOfRange);
}
TEST_F(NameTest, downcase) {
More information about the bind10-changes
mailing list