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

[BIND 10 Development]

#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      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Replying to [comment:10 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_;
 > }}}

 Good idea.. done. :)

 > - labelsequence should use uint8_t rather than unsigned char

 Done.

 > - same for messagenrenderer.

 Done.

 > - it looks like some other places in name.cc could use uint8_t instead

 Done in places where it's applicable. But there are still uses of unsigned
 char which comes from `Name()` accepting `std::string`.

 >   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);
 > }}}

 Agreed and fixed. Tests have been added for it (`make check` fails with
 `strncmp()` now).

 > - 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.

 This is because string literals are of type `(const char *)`. They need
 casting to unsigned, but instead of doing it in a dozen places, it is cast
 inside `getDataCheck()`.

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