Opened 10 years ago

Closed 10 years ago

#49 closed enhancement (fixed)

different signature for Name::split()

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: 04. 2nd Incremental Release: Early Adopters
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

Suggestion from Evan:
how about split(unsigned int first), which implicitly means (all labels from <first> to the end of the name)? so if n is "www.isc.org.", n.split(1) would be "isc.org."

Initial thought:
the usage seems reasonable: we'll probably often want to extract a specific "domain" part of an FQDN just like the above example.

how we realize this is an open question. There are several points to consider:

  • assuming we keep the more generic (current) version of split(), this can be implemented as a non member function. In general, non member functions are more preferable to member functions (as various C++ textbooks explain).
  • we may want to provide a bit generalized version of this feature, which would somehow return a pair of prefix and suffix separated from the original name at the specified point (e.g., return a pair of "www" and "example.com" for the above example). dnspython's split() method is implemented this way.
  • I expect we'll eventually need an even more generalized way to handle a sequence of labels (i.e., rather than constructing a splited name providing a "sequence" of labels contained in a name). if so, we may want to revisit the whole set of APIs.

Subtickets

Change History (13)

comment:1 Changed 10 years ago by jinmei

  • Milestone set to 04. 2nd Incremental Release
  • Priority changed from minor to major

comment:2 Changed 10 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing
  • Type changed from enhancement to review

trac #49 is ready for review.

Code is available on branches/trac49. Diff can be extracted by
svn diff -r r1808 svn+ssh://bind10.isc.org/svn/bind10/branches/trac49

(for convenience) an html version of doxgen document is available at:
http://bind10.isc.org/~jinmei/bind10/cpp/namespaceisc_1_1dns.html#ed7b8c11ca16460d4758a7ceb4921e7c
and
http://bind10.isc.org/~jinmei/bind10/cpp/namespaceisc_1_1dns.html#cc16cc549d992b4d4e8f914bca917661

I chose to implement them as non member functions rather than public
member functions of the name class. The rationale of this decision is
documented.

a minor note: diff to name.h contains some unrelated changes like
s/data/%data/. These are to avoid creating an automatic internal link
in the doxygen document.

comment:3 Changed 10 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from reviewing to accepted

comment:4 Changed 10 years ago by jreed

  • Type changed from review to enhancement

comment:5 Changed 10 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:6 Changed 10 years ago by shane

  • Owner changed from UnAssigned to shane
  • Status changed from reviewing to accepted

comment:7 Changed 10 years ago by shane

  • Owner changed from shane to jelte
  • Status changed from accepted to reviewing

comment:8 follow-up: Changed 10 years ago by jelte

  • Owner changed from jelte to jinmei

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>
}

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).

Apart from that, code looks good.

Does data really become a link in doxygen comments?

comment:9 in reply to: ↑ 8 Changed 10 years ago by jinmei

  • Owner changed from jinmei to jelte

Replying to 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.

comment:10 follow-up: Changed 10 years ago by jelte

  • Owner changed from jelte to jinmei

hmm. That whole data namespace is slated for complete removal :) (if that gets cancelled we should indeed rename it).

For the first, yes, I do, although it may have to do with the fact createAncestorNameOf and createSuffixNameOf don't really explain their difference, so in your example i'd have to look up what exactly it does, or perhaps that the --i there should probably be ++i :)

I also wouldn't really mind removing the other one, and perhaps this is something we can bring up at the meeting or on the list, if we can come up with method names that really tell the difference i'd be ok with having them both, and i wouldn't even mind having a createParentName() as well :)

comment:11 in reply to: ↑ 10 Changed 10 years ago by jinmei

  • Owner changed from jinmei to jelte

Replying to jelte:

As we discussed this morning/afternoon/evening, I dropped createAncestorNameOf and createSuffixNameOf and made the service of the former another signature of Name::split().

New doxygen document containing the design choice on this point is available at:
http://bind10.isc.org/~jinmei/bind10/cpp/classisc_1_1dns_1_1_name.html#a5ce58b92a8568a3f502affbdf8e0d606

It's r1886. Please check it.

comment:12 follow-up: Changed 10 years ago by jelte

  • Owner changed from jelte to jinmei

Looks good, ship it :)

comment:13 in reply to: ↑ 12 Changed 10 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to jelte:

Looks good, ship it :)

Thanks, committed (r1903), closing.

Note: See TracTickets for help on using tickets.