Adding a HexEncode function to string_utils....

Adding a HexEncode function to string_utils.
This function takes a pointer to a chunk of memory and formats the bytes as hex.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8420

[brettw, 2009-01-21 19:02:18]

http://codereview.chromium.org/18452/diff/5/8
File base/string_util.cc (right):

http://codereview.chromium.org/18452/diff/5/8#newcode1521
Line 1521: if (size > 0) {
I'd probably just do if (size == 0) return std::string(); to avoid the extra
layer of indenting in the function.

http://codereview.chromium.org/18452/diff/5/8#newcode1522
Line 1522: // for each byte, we print two characters
Make sure each comment begins with a capital and ends with a period.

[stoyan, 2009-01-21 19:04:30]

http://codereview.chromium.org/18452/diff/5/8
File base/string_util.cc (right):

http://codereview.chromium.org/18452/diff/5/8#newcode1515
Line 1515: static const char kHexChars[] = {
array is declared as 'char' but the initialization uses L prefixes for wchar_t

http://codereview.chromium.org/18452/diff/5/8#newcode1523
Line 1523: ret.resize(size * 2);
possible overflow?

http://codereview.chromium.org/18452/diff/5/7
File base/string_util.h (right):

http://codereview.chromium.org/18452/diff/5/7#newcode533
Line 533: std::string HexEncode(const void* bytes, int size);
I believe this should be 'size_t size'

[tommi (sloooow) - chröme, 2009-01-21 19:17:31]

also fixed new line issue (thanks maruel)

http://codereview.chromium.org/18452/diff/5/8
File base/string_util.cc (right):

http://codereview.chromium.org/18452/diff/5/8#newcode1515
Line 1515: static const char kHexChars[] = {
On 2009/01/21 19:04:30, stoyan wrote:
> array is declared as 'char' but the initialization uses L prefixes for wchar_t
good catch. I wrote the function as wide to begin with and then changed to
narrow.
fixed

http://codereview.chromium.org/18452/diff/5/8#newcode1521
Line 1521: if (size > 0) {
On 2009/01/21 19:02:18, brettw wrote:
> I'd probably just do if (size == 0) return std::string(); to avoid the extra
> layer of indenting in the function.

Done.

http://codereview.chromium.org/18452/diff/5/8#newcode1522
Line 1522: // for each byte, we print two characters
On 2009/01/21 19:02:18, brettw wrote:
> Make sure each comment begins with a capital and ends with a period.

Done.

http://codereview.chromium.org/18452/diff/5/8#newcode1523
Line 1523: ret.resize(size * 2);
added a DCHECK for that:

DCHECK(size < (std::numeric_limits<size_t>::max() / 2)) << " overflow";

[brettw, 2009-01-21 19:20:21]

http://codereview.chromium.org/18452/diff/5/8
File base/string_util.cc (right):

http://codereview.chromium.org/18452/diff/5/8#newcode1523
Line 1523: ret.resize(size * 2);
What kind of input is this being called on? If it's untrusted, we need to do a
CHECK rather than a DCHECK. If it's guaranteed to be trusted, I wouldn't worry
about checking for overflow here. The comment above the function should probably
document this assumption.

[Avi (use Gerrit), 2009-01-21 19:43:24]

http://codereview.chromium.org/18452/diff/207/15
File base/string_util.h (right):

http://codereview.chromium.org/18452/diff/207/15#newcode535
Line 535: #endif  // BASE_STRING_UTIL_H_
Lost the trailing newline

http://codereview.chromium.org/18452/diff/207/14
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/18452/diff/207/14#newcode1428
Line 1428: EXPECT_TRUE(hex.length() == 0);
Why not EXPECT_EQ(0, hex.length()), etc (and below, too?)

[tommi (sloooow) - chröme, 2009-01-21 19:52:56]

http://codereview.chromium.org/18452/diff/207/15
File base/string_util.h (right):

http://codereview.chromium.org/18452/diff/207/15#newcode535
Line 535: #endif  // BASE_STRING_UTIL_H_
On 2009/01/21 19:43:24, Avi wrote:
> Lost the trailing newline

there were two previously, I removed one... unless we want two?

http://codereview.chromium.org/18452/diff/207/14
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/18452/diff/207/14#newcode1428
Line 1428: EXPECT_TRUE(hex.length() == 0);
On 2009/01/21 19:43:24, Avi wrote:
> Why not EXPECT_EQ(0, hex.length()), etc (and below, too?)

Done.

[brettw, 2009-01-21 20:16:16]

http://codereview.chromium.org/18452/diff/18/19
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/18452/diff/18/19#newcode1428
Line 1428: EXPECT_EQ(hex.length(), 0);
I think your gcc problem is that you need to use "0U" since length() is
unsigned.

Also, the expected value should be the first argument. This way, the error
message will be correct.

[brettw, 2009-01-21 21:27:05]

LGTM

[Dean McNamee, 2009-01-22 11:22:03]

http://codereview.chromium.org/18452/diff/214/217
File base/string_util.cc (right):

http://codereview.chromium.org/18452/diff/214/217#newcode1515
Line 1515: static const char kHexChars[] = {
Could you use "0123456789ABCDEF" instead?

http://codereview.chromium.org/18452/diff/214/217#newcode1522
Line 1522: std::string ret;
Why not:

std::string ret(size * 2, '\0'); or something?

http://codereview.chromium.org/18452/diff/214/217#newcode1533
Line 1533: write[0] = kHexChars[(b >> 4) & 0xf];
I find write[0] strange if you're treating it like a pointer, why not *write = ?

I personally would have just used an index, and done
write[(i * 2)] = blah
write[(i *  2) + 1] = blah

or something