BIND 10 #404: serialize RDATA for in memory data source

[BIND 10 Development]

#404: serialize RDATA for in memory data source
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  task     |               Status:  accepted
                 Priority:  major    |            Milestone:
                Component:  data     |  Sprint-20110419
  source                             |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  0.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:15 vorner]:

 > I implemented most of the ideas. I left out the one with squeezing each
 FieldSpec into 2 bytes, as premature optimisation. We might want it in
 future, so I just put a TODO there. I did implement the removal of virtual
 methods. For one, it's less code and, for second, if they are already in
 one place, there's no need for virtual functions (at last for now).
 >
 > Can someone have a look?

 Now the review on the branch.

 '''messagerenderer'''

 - As I said in the previous comment, I'm okay with making most of the
 writeXXX() non virtual.  But the intent wouldn't be so clear (e.g., one
 would wonder why only these are non virtual while writeName is virtual).
 So I'd describe the design intent as a doxygen comment.
 - Also as I said, I'd still hide the buffer using pimpl at the moment.
 - In general I don't like to have a non private member variable unless
 it's absolutely necessary (just as many C++ books suggest), and in that
 sense I don't like to make AbstractMessageRenderer::buffer_ protected.
 IMO in this case it's not "absolute necessary", and I'd make it private
 (whether or not via pimpl) and have derived classes access it through
 public writeXXX interfaces. (I'm noticing that in order to do that we'd
 also need some extensions to AbstractMessageRenderer)
 - [comment] This is an off topic for this ticket, but I've been thinking
 it makes more sense to hide the output buffer inside the
 (Abstract)MessageRenderer (again, whether or not pimpl) rather than
 passing it via the constructor.  Previously the renderer class didn't have
 getData() and getDataLength(), so the caller needs to get access to the
 row data via the OutputBuffer interfaces.  But now the render also
 provides the way to get access to the raw data via its own
 getData()/getDataLength(), it makes the caller simpler if the renderer
 internally constructs the OutputBuffer (or even some other specialized
 data structure).  Now we are touching the renderer class, this may be a
 good opportunity to make this cleanup.  But since it will affect many
 other parts of the entire BIND 10 code (we'll also need to update the
 python binding and its users, for example), it may better be deffered to a
 separate ticket.  (If this change makes sense) either is okay for me.

 '''rdatafields.h'''
 - I don't understand the point of changing the return type of
 getFieldSpecData().  Which specific alias problem does the change try to
 solve?
 - In any case, I'd avoid offending comments like this:
 {{{
 +        // Nasty cast, because C++ authors didn't read the C
 specification
 +        // about pointers. We can't return void* from it, that would
 break
 }}}
   It's an unfounded accusation (how could we know whether C++ authors read
 the specification?), and even if it's the fact, this type of comment only
 makes us look vulgar and doesn't help anything IMO.  Let's be professional
 and just provide technical explanation.
 - And in any case, if the type change and cast are really necessary (which
 I didn't understand yet), I'd avoid using C-style cast.

 '''minor editorial points'''
 - In messagerenderer.h, 'at last' should be 'at least':
 {{{
 +    /// It was decided that there's no need to have this in every
 subclass,
 +    /// at last not now, and this reduces code size and gives compiler a
 better
 }}}
 - in rdatafields.cc, typo here also? (s/arriwe/arrive/?)
 {{{
     // So new data can arriwe without us knowing it, this considers all
 new
 }}}

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