BIND 10 #2384: extend RRTTL "from text" constructor so it can accpet 1H, 2D, etc.

[BIND 10 Development]

#2384: extend RRTTL "from text" constructor so it can accpet 1H, 2D, etc.
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121120
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  loadzone-ng                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:5 jinmei]:
 > I think it's worth a separate entry as it's an extension to a public
 > API (RRTTL class itself is public), but I'd leave it to you.

 OK, then. What about:

 {{{
 [func]          vorner
 TTLs can be specified with units as well as number of seconds now. This
 allows
 specifications like "1D3H".
 }}}

 > - maybe doesn't matter much, but the I wonder whether we want to avoid
 >   the for loop within while.  For example, if we use a helper function
 >   that returns `multiply` in a switch, we can avoid examining all
 >   possibility (and I assume such a function is small enough and
 >   inlined).

 I find the loop more readable than the switch. I don't like switches for
 bunch
 of identical cases.

 What is your concern here? Performance? Aside from the fact it would
 probably
 be premature, I think as the array is very small and the length is known
 at
 compile time, it'll get unrolled and optimised, possibly better than the
 switch.

 > - I also wonder whether the construction of the string could be
 >   slightly more efficient:
 > {{{#!cpp
 >
 boost::lexical_cast<int64_t>(string(pos,
 >
 unit));
 > }}}
 >   if we use ttlstr::substr(), assuming it uses reference counting copy
 >   (I guess the above version always allocates memory).  I'd also note
 >   that the construction always happens even if there's no unit and we
 >   could actually use the original ttlstr.

 I don't know if:
  * The strings actually do use reference counting (what happens when they
 are used across threads?).
  * There'd be any performance benefit. We are talking about very small
 amounts
    of memory for very short strings. Short-term allocation of such could
    possibly be faster than complicated code around reference counting.
  * The compiler couldn't do escape analysis, discover the string lives for
 a
    very short time and knowing its full implementation (as it is
 template),
    allocate on stack.
  * This is actually a place to optimise.

 For that reason, and because of the fact that using `::substr` would
 require
 integer offsets instead of iterators (which are returned by find), making
 the
 code more complicated, I don't really like the idea.

 Replying to [comment:6 jinmei]:
 > '''other'''
 > We need to update the documentation, explaining which format is
 > accepted, maybe also noting some awkward cases like "30M1H" or even
 > "1H2H".

 Done.

 > '''rrttl.cc'''
 >
 > - is static_cast needed?
 > {{{#!cpp
 >             const int64_t value = (unit == pos) ? 1 :
 >
 boost::lexical_cast<int64_t>(string(pos,
 >
 unit));
 > }}}
 >   For example, this implementation accepts "-1S1H".  Is it okay?
 >   How about this? "9223372036854775807S9223372036854775807S2S"
 >   (which would result in '0').  FYI: as far as I can see BIND 9 would
 >   reject it, although even BIND 9 doesn't seem to completely reject
 >   all overflow cases.

 It could be OK (because these are specially crafted cases abusing the
 knowledge
 of internals, and it is unlikely anyone would hit them by accident), but I
 added some more checking anyway.

 > - pretty minor, but I wonder whether we can avoid checking the
 >   condition of `pos != end` twice.  For example, if we eliminate the
 >   case of empty string, I guess the loop and condition could be:
 > {{{#!cpp
 >     while (true) {
 >         ...
 >         if (pos == end) {
 >             break;
 >         }
 >         ++pos;
 >     }
 > }}}

 Again, is the concern performance? Because I find this less readable (I
 like to
 know what is the end-condition for a cycle and looking it up through the
 body
 is not helpful). And I'm pretty confident the compiler can do something
 about
 it, again because strings are templates and it knows the internals. It can
 look
 and see the comparison is a pointer comparison anyway, without any side
 effects, optimising it out.

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