BIND 10 #2096: Define and implement "RdataReader" class

[BIND 10 Development]

#2096: Define and implement "RdataReader" class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120904
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  14
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:27 vorner]:

 > > {{{#!cpp
 > >     for (size_t i = 0; i < rdata_count; ++i) {
 > >          // write name, type, class, TTL
 > >          // skip space for RDATA len
 > >      reader.iterateRdata(); // must return true
 > >          // write RDATA len
 > >     }
 > > }}}
 >
 > If you're checking the reader.iterateRdata returns true every time
 (which you probably should, to check consistency), maybe you should check
 the next call after the loop returns false. Then it would have the
 positions computed already.

 Hmm, good point.  That's true.  I'd suggest adding some documentation
 about it though.  I'm attaching a proposed diff (where I fixed one
 other error in the doc: some parameters are missing for the
 `Reader` constructor in the code example.)

 > But I'm not against applying the patch anyway.

 With this clarification I'm okay with or without the code change.  I'd
 leave it to you.

 I'm okay with the points below open.  As you said it may be premature
 at this time, and we depend on this ticket for other tasks, so it's
 probably better to move forward sooner.

 Finally, please don't forget
 http://bind10.isc.org/ticket/2096?replyto=27#comment:22

 And then feel free to merge.

 > > Further, this makes me wonder whether we want to keep the behavior of
 > > next().  At the moment, I don't see the need for it: we only need
 > > iterate() (for additional section handling) and iterateRdata() (for
 > > `RRset` rendering).  And, since we implement these based on the next()
 > > behavior, these need additional layer of function calls for each
 > > single "data field".  I wonder whether we could just expand the
 > > `nextInternal` behavior so it examines all fields of each RDATA,
 > > reducing the additional layer.
 >
 > Maybe we don't need it now. Maybe sometime in future, it could come
 handy. So I'll ask differently ‒ is there any good reason to spend the
 time removing next() from the tests and changing it? If it's performance,
 I think it's premature and really minor (this is a call to fixed address
 in the code, if the compiler does not optimise it out completely, compared
 with a call with boost::bind and boost::function in the callback, which is
 at least one virtual call, probably flushing the processor pipeline,
 etc…).
 >
 > Thank you

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