[1602] Address review comments

- split up split() into leftSplit() and rightSplit()
- made LabelSequence a Friend class of Name
   - no need for separate offsets vector
   - no need for Name::at_p()
- fixed copyright year :)

src/lib/dns/labelsequence.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2009  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -19,18 +19,6 @@
 namespace isc {
 namespace dns {
 
-LabelSequence::LabelSequence(const Name& name) : name_(name),
-                             first_label_(0), offsets_(name.getLabelCount()) {
-    offsets_[0] = 0;
-    last_label_ = name.getLabelCount();
-    // Walk through the wire format data and store all offsets
-    for (size_t i = 1; i < last_label_; ++i) {
-        // Each offset is the previous offset plus the length of the
-        // label plus 1 (for the label length octet)
-        offsets_[i] = offsets_[i - 1] + name.at(offsets_[i - 1]) + 1;
-    }
-}
-
 const char*
 LabelSequence::getData(size_t *len) const {
     // If the labelsequence is absolute, the current last_label_ falls
@@ -39,11 +27,11 @@ LabelSequence::getData(size_t *len) const {
     // the length for the 'previous' label (the root label) plus
     // one (for the root label zero octet)
     if (isAbsolute()) {
-        *len = offsets_[last_label_ - 1] - offsets_[first_label_] + 1;
+        *len = name_.offsets_[last_label_ - 1] - name_.offsets_[first_label_] + 1;
     } else {
-        *len = offsets_[last_label_] - offsets_[first_label_];
+        *len = name_.offsets_[last_label_] - name_.offsets_[first_label_];
     }
-    return (name_.at_p(offsets_[first_label_]));
+    return (&name_.ndata_[name_.offsets_[first_label_]]);
 }
 
 bool
@@ -63,21 +51,26 @@ LabelSequence::equals(const LabelSequence& other, bool case_sensitive) const {
 }
 
 void
-LabelSequence::split(int i) {
-    if (abs(i) >= getLabelCount()) {
+LabelSequence::leftSplit(size_t i) {
+    if (i >= getLabelCount()) {
         isc_throw(OutOfRange, "Cannot split to zero or less labels; " << i <<
                               " (labelcount: " << getLabelCount() << ")");
     }
-    if (i > 0) {
-        first_label_ += i;
-    } else if (i < 0) {
-        last_label_ += i;
+    first_label_ += i;
+}
+
+void
+LabelSequence::rightSplit(size_t i) {
+    if (i >= getLabelCount()) {
+        isc_throw(OutOfRange, "Cannot split to zero or less labels; " << i <<
+                              " (labelcount: " << getLabelCount() << ")");
     }
+    last_label_ -= i;
 }
 
 bool
 LabelSequence::isAbsolute() const {
-    return (last_label_ == offsets_.size());
+    return (last_label_ == name_.offsets_.size());
 }
 
 } // end namespace dns

src/lib/dns/labelsequence.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2009  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -49,7 +49,10 @@ public:
     /// to the labels in the Name object).
     ///
     /// \param name The Name to construct a LabelSequence for
-    LabelSequence(const Name& name);
+    LabelSequence(const Name& name): name_(name),
+                                     first_label_(0),
+                                     last_label_(name.getLabelCount())
+    {}
 
     /// \brief Return the wire-format data for this LabelSequence
     ///
@@ -76,20 +79,27 @@ public:
     ///         and contain the same data.
     bool equals(const LabelSequence& other, bool case_sensitive = false) const;
 
-    /// \brief Remove one or more labels from this LabelSequence
+    /// \brief Remove labels from the front of this LabelSequence
     ///
-    /// Removes labels from either the front or the back of the LabelSequence
+    /// \note No actual memory is changed, this operation merely updates the
+    /// internal pointers based on the offsets in the Name object.
+    ///
+    /// \exeption OutOfRange if i is greater than or equal to the number
+    ///           of labels currently pointed to by this LabelSequence
+    ///
+    /// \param i The number of labels to remove.
+    void leftSplit(size_t i);
+
+    /// \brief Remove labels from the end of this LabelSequence
     ///
     /// \note No actual memory is changed, this operation merely updates the
-    /// internal pointers based on the offsets at creation time.
+    /// internal pointers based on the offsets in the Name object.
     ///
-    /// \exeption OutOfRange if abs(i) is greater than or equal to the
-    ///           number of labels currently pointed to by this LabelSequence
+    /// \exeption OutOfRange if i is greater than or equal to the number
+    ///           of labels currently pointed to by this LabelSequence
     ///
-    /// \param i When positive, removes i labels from the front of the
-    ///        LabelSequence. When negative, removes i labels from the
-    ///        end of it. When zero, this is a no-op.
-    void split(int i);
+    /// \param i The number of labels to remove.
+    void rightSplit(size_t i);
 
     /// \brief Returns the current number of labels for this LabelSequence
     ///
@@ -116,7 +126,6 @@ private:
     const Name& name_;
     size_t first_label_;
     size_t last_label_;
-    std::vector<size_t> offsets_;
 };
 
 

src/lib/dns/name.h

@@ -210,6 +210,11 @@ private:
 /// names as a special case.
 ///
 class Name {
+    // LabelSequences use knowledge about the internal data structure
+    // of this class for efficiency (they use the offsets_ vector and
+    // the ndata_ string)
+    friend class LabelSequence;
+
     ///
     /// \name Constructors and Destructor
     ///
@@ -299,34 +304,6 @@ public:
         return (ndata_[pos]);
     }
 
-    ///
-    /// \brief Provides access to the memory pointer of the wire format at
-    /// the specified position
-    ///
-    /// This method is similar to Name::at(size_t pos), but instead of
-    /// the value at the given position, it returns the memory address
-    /// (as a const char*).
-    /// This is only validly usable as long as the Name object is in scope
-    /// and does not change, so care must be taken when using the value
-    /// returned by this method.
-    /// For example use of this method, see the LabelSequence class. Outside
-    /// of that class, it is advisable not to use raw data as exposed here.
-    ///
-    /// \exception OutOfRange thrown if \c pos is higher than the wire format
-    ///            length of the Name data
-    ///
-    /// \param pos The position in the wire format name %data to be returned.
-    /// \return A char* corresponding to the address of the data at the
-    /// position of \c pos.
-    const char* at_p(size_t pos) const
-    {
-        if (pos >= length_) {
-            isc_throw(OutOfRange, "Out of range access in Name::at_p(): " <<
-                                  pos << " > " << length_);
-        }
-        return (&ndata_[pos]);
-    }
-
     /// \brief Gets the length of the <code>Name</code> in its wire format.
     ///
     /// This method never throws an exception.

src/lib/dns/tests/labelsequence_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2009  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -143,77 +143,77 @@ TEST_F(LabelSequenceTest, getData) {
     getDataCheck("\000", 1, ls7);
 };
 
-TEST_F(LabelSequenceTest, split_pos) {
+TEST_F(LabelSequenceTest, leftSplit) {
     EXPECT_TRUE(ls1.equals(ls3));
-    ls1.split(0);
+    ls1.leftSplit(0);
     getDataCheck("\007example\003org\000", 13, ls1);
     EXPECT_TRUE(ls1.equals(ls3));
-    ls1.split(1);
+    ls1.leftSplit(1);
     getDataCheck("\003org\000", 5, ls1);
     EXPECT_FALSE(ls1.equals(ls3));
-    ls1.split(1);
+    ls1.leftSplit(1);
     getDataCheck("\000", 1, ls1);
     EXPECT_TRUE(ls1.equals(ls7));
 
-    ls2.split(2);
+    ls2.leftSplit(2);
     getDataCheck("\000", 1, ls2);
     EXPECT_TRUE(ls2.equals(ls7));
 }
 
-TEST_F(LabelSequenceTest, split_neg) {
+TEST_F(LabelSequenceTest, rightSplit) {
     EXPECT_TRUE(ls1.equals(ls3));
-    ls1.split(-1);
+    ls1.rightSplit(1);
     getDataCheck("\007example\003org", 12, ls1);
     EXPECT_FALSE(ls1.equals(ls3));
-    ls1.split(-1);
+    ls1.rightSplit(1);
     getDataCheck("\007example", 8, ls1);
     EXPECT_FALSE(ls1.equals(ls3));
 
     ASSERT_FALSE(ls1.equals(ls2));
-    ls2.split(-2);
+    ls2.rightSplit(2);
     getDataCheck("\007example", 8, ls2);
     EXPECT_TRUE(ls1.equals(ls2));
 }
 
 TEST_F(LabelSequenceTest, split_oor) {
-    EXPECT_THROW(ls1.split(100), isc::OutOfRange);
-    EXPECT_THROW(ls1.split(5), isc::OutOfRange);
-    EXPECT_THROW(ls1.split(4), isc::OutOfRange);
-    EXPECT_THROW(ls1.split(3), isc::OutOfRange);
+    EXPECT_THROW(ls1.leftSplit(100), isc::OutOfRange);
+    EXPECT_THROW(ls1.leftSplit(5), isc::OutOfRange);
+    EXPECT_THROW(ls1.leftSplit(4), isc::OutOfRange);
+    EXPECT_THROW(ls1.leftSplit(3), isc::OutOfRange);
     getDataCheck("\007example\003org\000", 13, ls1);
 
-    EXPECT_THROW(ls1.split(-100), isc::OutOfRange);
-    EXPECT_THROW(ls1.split(-5), isc::OutOfRange);
-    EXPECT_THROW(ls1.split(-4), isc::OutOfRange);
-    EXPECT_THROW(ls1.split(-3), isc::OutOfRange);
+    EXPECT_THROW(ls1.rightSplit(100), isc::OutOfRange);
+    EXPECT_THROW(ls1.rightSplit(5), isc::OutOfRange);
+    EXPECT_THROW(ls1.rightSplit(4), isc::OutOfRange);
+    EXPECT_THROW(ls1.rightSplit(3), isc::OutOfRange);
     getDataCheck("\007example\003org\000", 13, ls1);
 }
 
 TEST_F(LabelSequenceTest, getLabelCount) {
     EXPECT_EQ(3, ls1.getLabelCount());
-    ls1.split(0);
+    ls1.leftSplit(0);
     EXPECT_EQ(3, ls1.getLabelCount());
-    ls1.split(1);
+    ls1.leftSplit(1);
     EXPECT_EQ(2, ls1.getLabelCount());
-    ls1.split(1);
+    ls1.leftSplit(1);
     EXPECT_EQ(1, ls1.getLabelCount());
 
     EXPECT_EQ(3, ls2.getLabelCount());
-    ls2.split(-1);
+    ls2.rightSplit(1);
     EXPECT_EQ(2, ls2.getLabelCount());
-    ls2.split(-1);
+    ls2.rightSplit(1);
     EXPECT_EQ(1, ls2.getLabelCount());
 
     EXPECT_EQ(3, ls3.getLabelCount());
-    ls3.split(-2);
+    ls3.rightSplit(2);
     EXPECT_EQ(1, ls3.getLabelCount());
 
     EXPECT_EQ(5, ls4.getLabelCount());
-    ls4.split(-3);
+    ls4.rightSplit(3);
     EXPECT_EQ(2, ls4.getLabelCount());
 
     EXPECT_EQ(3, ls5.getLabelCount());
-    ls5.split(2);
+    ls5.leftSplit(2);
     EXPECT_EQ(1, ls5.getLabelCount());
 }
 
@@ -221,11 +221,11 @@ TEST_F(LabelSequenceTest, comparePart) {
     EXPECT_FALSE(ls1.equals(ls8));
 
     // Split root label from example.org.
-    ls1.split(-1);
+    ls1.rightSplit(1);
     // Split foo from foo.example.org.bar.
-    ls8.split(1);
+    ls8.leftSplit(1);
     // Split bar. (i.e. bar and root) too
-    ls8.split(-2);
+    ls8.rightSplit(2);
 
     EXPECT_TRUE(ls1.equals(ls8));
 
@@ -238,16 +238,16 @@ TEST_F(LabelSequenceTest, comparePart) {
 TEST_F(LabelSequenceTest, isAbsolute) {
     ASSERT_TRUE(ls1.isAbsolute());
 
-    ls1.split(1);
+    ls1.leftSplit(1);
     ASSERT_TRUE(ls1.isAbsolute());
-    ls1.split(-1);
+    ls1.rightSplit(1);
     ASSERT_FALSE(ls1.isAbsolute());
 
     ASSERT_TRUE(ls2.isAbsolute());
-    ls2.split(-1);
+    ls2.rightSplit(1);
     ASSERT_FALSE(ls2.isAbsolute());
 
     ASSERT_TRUE(ls3.isAbsolute());
-    ls3.split(2);
+    ls3.leftSplit(2);
     ASSERT_TRUE(ls3.isAbsolute());
 }