[bind10-dev] Name::reverse()?

[JINMEI Tatuya / 神明達哉]

(As Evan asked me, and in an attempt of not burying technical
discussions in the poor memory of jabber log, I'm sending this to
bind10-dev)

I've given a quick and unofficial review of the Name::reverse()
implementation (svn rev 877)
http://bind10.isc.org/browser/branches/each-ds/src/lib/dns/cpp/name.cc?rev=877

I have some minor comments on the implementations and have a counter
proposal patch (copied below):

- I'd use string::append instead of string::insert, and overall
  simplify the loop logic
- I'd build the reverse offset vector directly from the original
  offsets using transform().  It should also help simplify the code
  logic (by eliminating the boundary case consideration for
  offsets_[0])
- I'd use ++x instead of x++.  In C++ the former is generally
  preferable; although in this specific case this may not make a
  significant advantage, but for consistency I'd suggest we should use
  the former unless we have a specific reason to use the latter.

More important point is that I'm not convinced about

1. whether we need to provide this functionality as part of the public
   DNS library
2. whether we should implement this as a member function

Currently, (AFAICS) the only usage of this method is the following
line of data_source_sqlite3.cc:

    const char *c_rname = qname.reverse().toText().c_str();

This seems to me a very special case.  Could that have more generic
use cases that justify the addition to the public library?  Basically
I wouldn't oppose to make the library convenient, but I don't like to
make it a set of kitchen sink features for very minor purposes.

I've quickly reviewed other similar libraries: none of dnspython,
dnsruby or Perl Net::DNS has this type of feature.  But, hmm, ldns
seems to have it:
http://www.nlnetlabs.nl/projects/ldns/doc/dname_8c.html#a5fe17a6b657bca88b134c228671ea36c

I don't know whether this function is widely used.  The only user
within ldns itself is drill, which (seemingly) uses this function to
build an in-addr.arpa or ip6.arpa name from textual an IPv4/IPv6
addresses.  But it doesn't seem to have to be a name reverse function,
as far as I can see...

Second, In some cases, we may still want to have relatively minor
functionality in the public library.  But especially for minor usage,
I basically think we should avoid adding class member functions so
that we won't make the class too monolithic.

---
JINMEI, Tatuya


Index: name.cc
===================================================================
--- name.cc	(revision 878)
+++ name.cc	(working copy)
@@ -618,19 +618,22 @@
     retname.offsets_.reserve(labelcount_);
     retname.ndata_.reserve(length_);
 
-    std::vector<unsigned char>::const_reverse_iterator rit;
-    rit = offsets_.rbegin();
-    rit++;  // skip over the trailing dot
-    retname.offsets_.push_back(0);
-    while (rit < offsets_.rend()) {
-        unsigned char offset = *rit;
-        retname.ndata_.insert(retname.ndata_.size(), ndata_, offset,
-                              ndata_.at(offset) + 1);
-        retname.offsets_.push_back(retname.ndata_.size());
-        rit++;
+    // Copy the original name, label by label, from tail to head.
+    vector<unsigned char>::const_reverse_iterator rit0 = offsets_.rbegin();
+    vector<unsigned char>::const_reverse_iterator rit1 = rit0 + 1;
+    string::const_iterator n0 = ndata_.begin();
+    while (rit1 != offsets_.rend()) {
+        retname.ndata_.append(n0 + *rit1, n0 + *rit0);
+        ++rit0;
+        ++rit1;
     }
+    retname.ndata_.push_back(0);
 
-    retname.ndata_.push_back(0);
+    // Build the reverse offset from the original offset.
+    transform(offsets_.rbegin(), offsets_.rend(),
+              back_inserter(retname.offsets_),
+              bind1st(minus<unsigned char>(), length_ - 1));
+
     retname.labelcount_ = labelcount_;
     retname.length_ = length_;