BIND 10 #1774: use uint8_t for "characters" of Name data

BIND 10 Development do-not-reply at isc.org
Fri Jun 22 21:30:13 UTC 2012


#1774: use uint8_t for "characters" of Name data
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120703
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  libdns++                           |  Estimated Difficulty:  3
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 - maybe a matter of taste, especially because it's class-private data,
   but I'd personally define a custom type instead of directly using
   `string<uint8_t>`:
 {{{#!cpp
     typedef std::basic_string<uint8_t> NameString;
     NameString ndata_;
 }}}
 - labelsequence should use uint8_t rather than unsigned char
 - same for messagenrenderer.
 - it looks like some other places in name.cc could use uint8_t instead
   of unsigned char
 - this doesn't seem to be correct in various points:
 {{{#!cpp
         return (std::strncmp((const char*) data,
                              (const char*) other_data,
                              len) == 0);

 }}}
   First, although this is not because of this branch, I suspect
   str[n]cmp cannot be used here because data/other_data can contain \0
   as part of a valid label.  I also suggest adding a specific test
   that can either confirm or deny this.  And, in any case, memcmp is a
   better choice here, and probably the only correct choice if my
   suspicion is correct.  Third, we shouldn't deceive compiler by cast
   in this type of case.  Fourth, even if this were one of such rare
   cases, we should never use C-style cast.  Combining these, this part
   should be:
 {{{#!cpp
         // and type of data/other_data should be changed to uint8_t
         return (std::memcmp(data, other_data, len) == 0);
 }}}
 - same sense of comment applies to labelsequence test.  I'd first
   clarify the type of getData() and test data, too.  On top of that
   I'd make overall cleanup.  If it still seems to require casting from
   unsigned char* to char*, something should be wrong.

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


More information about the bind10-tickets mailing list