BIND 10 #49: different signature for Name::split()

[BIND 10 Development]

#49: different signature for Name::split()
---------------------------+------------------------------------------------
 Reporter:  jinmei         |        Owner:  jelte                      
     Type:  enhancement    |       Status:  reviewing                  
 Priority:  major          |    Milestone:  04. 2nd Incremental Release
Component:  DNSPacket API  |   Resolution:                             
 Keywords:                 |    Sensitive:  0                          
---------------------------+------------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => jelte


Comment:

 Replying to [comment:8 jelte]:
 > I'm not entirely sure how useful createSuffixNameOf() is; i'd just as
 easily write
 >
 {{{
 for(i = label.getLabelCount(); i>0; --i) {
         Name n(createAncestorOf(label));
         <etc>
 }
 }}}

 I in fact didn't like to provide different specialized functions for
 mostly the same purpose, but chose this approach after considering how to
 improve readability of the for loop of data_soruce.cc:hasDelegation().  It
 currently reads:

 {{{
 const int nlen = task->qname.getLabelCount();
 const int diff = nlen - zonename->getLabelCount();
 if (diff > 1) {
     for (int i = diff; i > 1; --i) {
         const Name sub(task->qname.split(i - 1, nlen - i));
             ...
     }
 }
 }}}

 I'm pretty sure no one can understand what this code does without drawing
 some pictures:-)

 With createSuffixNameOf(), we could convert it as follows:

 {{{
 if (task->qname.getLabelCount() > zonename->getLabelCount()) {
     for (int i = zonename->getLabelCount() + 1;
          i <= task->qname.getLabelCount();
          --i) {
         const Name sub(createSuffixNameOf(task->qname, i));
         ...
     }
     ....
 }
 }}}

 which I believe is much clearer.

 With createAncestorNameOf(), it would be as follows:

 {{{
 const int nlen = task->qname.getLabelCount();
 const int diff = nlen - zonename->getLabelCount();
 if (diff > 1) {
     for (int i = diff - 1; i >= 0; --i) {
         const Name sub(createAncestorNameOf(task->qname, i));
         ...
     }
     ...
 }
 }}}

 ...hmm, now it doesn't look that bad to me...although the logic should be
 clearer with createSuffixNameOf(), it may not be so substantial that the
 redundancy can be justified.

 Do you still think it's sufficient to have only createAncestorNameOf()
 with these specific examples?  If so, I think I should accept that
 comment.

 > and we'd have to look up which one did what every time we used it (since
 the names don't really make that clear to me)...
 >
 > (to be completely honest, i wouldn't even mind adding a second
 > name::split(int) for this, but have no objections to the current
 > approach).

 Yeah, naming is another problem.  I admit these names may not be best
 choices.  If you have suggestions I'm glad to hear.

 Alternatively, if the consensus on the first point (one vs two functions)
 is to drop the idea of "suffix", this may be a stronger reason for making
 it a method with a single argument: Name::split(int level);  If we had two
 more methods, it might not be so clear about which one is which just from
 the signature, but if we only have one more it's probably okay.  Would you
 prefer that apporach?

 > Apart from that, code looks good.

 Okay, thanks.

 > Does data really become a link in doxygen comments?

 Yes, it does.  It's due to the isc::data name space.  See, for example,
 the "detailed description" part of
 http://bind10.isc.org/~jinmei/bind10/cpp/classisc_1_1dns_1_1_r_rset.html

 Actually, I have a minor concern related on this point.  "data" may be too
 generic for the name space of a single module of one application (= BIND
 10).  The weird doxygen link is itself a minor symptom of this naming, but
 I suspect it may cause more substantial conflicts and confusion.

 Maybe we should rename it to something bit more specific, e.g. imc_data
 (imc = "inter module communication")? - yeah I know it's not so sexy
 either.

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