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:9 jinmei]:
 > - I didn't understand the intent of the first paragraph of the
 >   comment.  You mean "Check if the partial value..."?   I didn't
 >   understand what this means either: "if we get out now, it won't get
 >   better"

 I tried to make it clearer.

 > - is the 10-digit check necessary?  at least the current set of tests
 >   don't require this check in my environment (is that related to the
 >   comment about lexical_cast?).  Isn't it enough if `value`,
 >   `multiply * value` and `val` are all uint32_t numbers?

 I was worried about the int64_t wrapping around silently. But it seems the
 lexical_cast throws here too. So I'll just assume it throws every time,
 there
 is a test for the case, so if it doesn't work everywhere, we'll know about
 it.

 > - Also, is `unit - pos` safe?  `operator-` only works for random
 >   access iterators, and (in my understanding) we cannot assume the
 >   type of string iterators.

 It is not used any more. But I think it is. The string has `operator[]`
 and
 `cstr()`, which both indicate string is random-accessible and the internal
 representation is just a block of memory. These are just hints, I'm not
 sure it
 is actually mandated explicitly.

 > '''About performance related comments'''
 >
 > I'm basically okay with skipping my points, but I still suggest
 > considering a special case of digit-only TTLs.  From my quick
 > benchmark if we simply do something like this:
 > {{{#!cpp
 >             const string::const_iterator unit = find_if(pos, end,
 myIsalpha);
 >             if (unit == end && pos == beg) { // beg = ttlstr.begin()
 >                 value = boost::lexical_cast<int64_t>(ttlstr);
 >                 break;
 >             }
 > }}}
 > it's 40% faster than the current version for the input of "3600".
 > I'm attaching the benchmark code in case you're interested.

 I'm still not sure it would be significant (it's 40% of TTL parsing, but I
 guess the TTL parsing takes a really small part of the loading). Anyway,
 it
 turned out to be possible without introducing anything complicated,
 together
 with the disallowing of missing last unit.

 > - This case is not allowed in BIND 9:
 > {{{#!cpp
 >     /// The
 >     /// unit at the last number can be omitted in case it is seconds.
 > }}}
 >   and in any case it doesn't seem to be tested.

 It was tested (see the removed test in
 45d598bed63a8d036d821085310a6ab1f5d45434). But it's not interesting any
 more.

 > - In test, this test case is duplicated:
 > {{{#!cpp
 >     EXPECT_EQ(4294967295, RRTTL("4294967295").getValue());
 > }}}

 It's not. One is with S at the end, it tests the max value works both in
 units and non-units mode.

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