BIND 10 #256: bugs in base32.cc

[BIND 10 Development]

#256: bugs in base32.cc
--------------------------------+-------------------------------------------
      Reporter:  jinmei         |        Owner:  jinmei                     
          Type:  defect         |       Status:  reviewing                  
      Priority:  major          |    Milestone:  06. 4th Incremental Release
     Component:  DNSPacket API  |   Resolution:                             
      Keywords:                 |    Sensitive:  0                          
Estimatedhours:                 |        Hours:                             
      Billable:  1              |   Totalhours:                             
      Internal:  0              |  
--------------------------------+-------------------------------------------
Changes (by stephen):

  * owner:  stephen => jinmei


Comment:

 > '''dns/util/base_n.cc'''
 > :
 > I've thought about it, and chose to just make comments on the assumption
 without modifying the
 > code. As you also noted, these are only intended to be used within this
 file, each used only
 > once, so it wouldn't be much beneficial to try to validate the arguments
 100%. Also, if we make
 > it very sure they are valid, we should also think about other invalid
 cases such as base_end
 > beyond the end of the valid range, base_beginpad_ > base_end_, etc.
 Making these perfectly
 > correct would complicate the resulting code, and I suspect the accuracy
 isn't worth the
 > complexity in this case.

 Fair enough.  No need to make any further modifications here. If we use
 the code elsewhere though, we should revisit it.  I was thinking that
 these iterators could be useful in other circumstances where handling
 transformations to and from base-n encoded text is required. There
 shouldn't be too much extra effort, as the iterator checks can only be
 simple - unless an iterator is a random-access iterator it is not possible
 to compare iterators for anything other than equality.


 > I follow one common (but probably not so widely deployed) convetion: use
 struct for stateless
 > class, and use class if it maintains states. But, admittedly, I'm not
 sure if this separation
 > is helpful, and I also suspect I'm not always consistent in this
 separation. If you prefer
 > consistency, I don't mind changing BaseNTransformer to a class.

 I've not come across that one.  My own use has been to use "struct" where
 it comprises only data members (with, perhaps, a constructor) and "class"
 where it has methods.  But that is really a hangover from C programming.
 As "class" and "struct" are equivalent (apart from the default visibility
 of members), it doesn't really matter apart from the look of the thing.
 Leave it as it is.


 >> In the calculation of padbits in BaseNTransformer::decode, the use of
 "& ~7" is a neat trick
 >> to round down to the nearest multiple of 8. It took me a few seconds to
 work out why it was
 >> there though - perhaps a comment would be in order?
 > I agree this is not trivial. Added more comments.

 The "& ~7" still needs a brief comment (e.g. "the bitwise AND with ~7
 clears the lowest three bits, so has the effect of rounding the result
 down to the nearest multiple of 8").  I only mentioned this as it's non-
 obvious so it's something I think a reader would look at and puzzle over
 until they work it out. (Well, I did!)


 > '''src/lib/dns/util/README'''
 > :
 > Good point, I tried to clarify it in a revised README (r2539). Actually,
 I'm not 100% sure
 > this is an agreed development policy. Maybe we should discuss it in the
 weekly call.

 I think that would be a useful discussion.

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