Remove some uses of isdigit in net/, as it can be locale depedent.

Remove some uses of isdigit in net/, as it can be locale depedent.

BUG=671813
Committed: https://crrev.com/0f8ee2261a801a18d55cab7f28d5551ca23ed012
Cr-Commit-Position: refs/heads/master@{#437045}

[mmenke, 2016-12-06 22:43:40]

Patchset #2 (id:20001) has been deleted

[mmenke, 2016-12-06 22:44:08]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:44:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-12-06 22:56:40]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:57:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 00:48:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 00:48:16]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-07 16:23:08]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org

[mmenke, 2016-12-07 16:24:37]

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
File net/spdy/spdy_alt_svc_wire_format.cc (right):

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
net/spdy/spdy_alt_svc_wire_format.cc:359: return
ParsePositiveIntegerImpl<uint32_t>(c, end, value);
Could use the parse_number method here, but we'd have to create a StringPiece
using the knowledge that StringPiece::const_iterators are just pointers to a C++
character array.

[eroman, 2016-12-07 18:40:12]

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
File net/spdy/spdy_alt_svc_wire_format.cc (right):

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
net/spdy/spdy_alt_svc_wire_format.cc:271: bool
SpdyAltSvcWireFormat::PercentDecode(StringPiece::const_iterator c,
Hmm, weird.
When I think percent escaping I think URL style (with decimal digits).

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
net/spdy/spdy_alt_svc_wire_format.cc:293: decoded += base::HexDigitToInt(*c);
Hmm.

Signed overflow is undefined, so this addition is potentially undefined behavior
(as is the initialization of decoded).

Not sure why C makes this stuff so unbelievably difficult.

 * char is not guaranteed to be either signed or unsigned
 * the expression (myuint8 << 4) returns an int (SIGNED int), because hey, why
not? (making narrowing and signed/unsigned compilation warnings all the more
noisy)

Probably the most sane thing to do is just make decoded be an |int|.

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
net/spdy/spdy_alt_svc_wire_format.cc:359: return
ParsePositiveIntegerImpl<uint32_t>(c, end, value);
On 2016/12/07 16:24:36, mmenke wrote:
> Could use the parse_number method here, but we'd have to create a StringPiece
> using the knowledge that StringPiece::const_iterators are just pointers to a
C++
> character array.

Could also do something like this:

StringPiece MakeStringPiece(StringPiece::const_iterator begin,
StringPiece::const_iterator end) {
  auto length = std::distance(begin, end);
  return length <= 0 ? StringPiece() : StringPiece(&(*begin),
static_cast<size_t>(length));
}


....

return ParseUint32(MakeStringPiece(...), value) && value > 0;


And for the 16-bit version I would be fine with adding a 16-bit flavor to
parse_number.h (there are at least 1 or 2 places that could use that too,
dealing with port parsing IIRC).

[mmenke, 2016-12-07 19:02:51]

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
File net/spdy/spdy_alt_svc_wire_format.cc (right):

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
net/spdy/spdy_alt_svc_wire_format.cc:271: bool
SpdyAltSvcWireFormat::PercentDecode(StringPiece::const_iterator c,
On 2016/12/07 18:40:11, eroman (slow) wrote:
> Hmm, weird.
> When I think percent escaping I think URL style (with decimal digits).

decimal digits?  I'm not following.  URLs use hex escaping, too.

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
net/spdy/spdy_alt_svc_wire_format.cc:293: decoded += base::HexDigitToInt(*c);
On 2016/12/07 18:40:11, eroman (slow) wrote:
> Hmm.
> 
> Signed overflow is undefined, so this addition is potentially undefined
behavior
> (as is the initialization of decoded).
> 
> Not sure why C makes this stuff so unbelievably difficult.
> 
>  * char is not guaranteed to be either signed or unsigned
>  * the expression (myuint8 << 4) returns an int (SIGNED int), because hey, why
> not? (making narrowing and signed/unsigned compilation warnings all the more
> noisy)
> 
> Probably the most sane thing to do is just make decoded be an |int|.

SGTM, done, though still have to cast the int to char.

[eroman, 2016-12-07 19:11:53]

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
File net/spdy/spdy_alt_svc_wire_format.cc (right):

https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
net/spdy/spdy_alt_svc_wire_format.cc:271: bool
SpdyAltSvcWireFormat::PercentDecode(StringPiece::const_iterator c,
On 2016/12/07 19:02:50, mmenke wrote:
> On 2016/12/07 18:40:11, eroman (slow) wrote:
> > Hmm, weird.
> > When I think percent escaping I think URL style (with decimal digits).
> 
> decimal digits?  I'm not following.  URLs use hex escaping, too.

My mistake, realized right after sending :)

I assumed there was a difference or this would be using a shared url-unescaping
routine.
But perhaps the only difference is lack of support for wide strings?

[eroman, 2016-12-07 19:13:55]

lgtm

[mmenke, 2016-12-07 19:14:38]

On 2016/12/07 19:11:53, eroman (slow) wrote:
>
https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
> File net/spdy/spdy_alt_svc_wire_format.cc (right):
> 
>
https://codereview.chromium.org/2558643002/diff/60001/net/spdy/spdy_alt_svc_w...
> net/spdy/spdy_alt_svc_wire_format.cc:271: bool
> SpdyAltSvcWireFormat::PercentDecode(StringPiece::const_iterator c,
> On 2016/12/07 19:02:50, mmenke wrote:
> > On 2016/12/07 18:40:11, eroman (slow) wrote:
> > > Hmm, weird.
> > > When I think percent escaping I think URL style (with decimal digits).
> > 
> > decimal digits?  I'm not following.  URLs use hex escaping, too.
> 
> My mistake, realized right after sending :)
> 
> I assumed there was a difference or this would be using a shared
url-unescaping
> routine.
> But perhaps the only difference is lack of support for wide strings?

I thought the difference was that this takes a StringPiece::const_iterator,
instead of a string or StringPiece.  Doesn't look like this supports wide
strings.

[mmenke, 2016-12-07 19:14:53]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-12-07 19:15:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2016-12-07 19:19:01]

right, our support in src/url is to inflate to a wide strings
https://cs.chromium.org/chromium/src/url/url_util.h?q=url/url_util.h&sq=packa...

[mmenke, 2016-12-07 19:20:49]

On 2016/12/07 19:19:01, eroman (slow) wrote:
> right, our support in src/url is to inflate to a wide strings
>
https://cs.chromium.org/chromium/src/url/url_util.h?q=url/url_util.h&sq=packa...

We have string and string16 variants of UnescapeURLComponent in net/, though.

[commit-bot: I haz the power, 2016-12-07 20:15:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481138093017560,
"parent_rev": "485391a99cf272d989dee57e8f67f597e7c1d13d", "commit_rev":
"47c580cf983ba200fadcf9a3550d368e1e439f30"}

[commit-bot: I haz the power, 2016-12-07 20:16:04]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-07 20:19:25]

Description was changed from

==========
Remove some uses of isdigit in net/, as it can be locale depedent.

BUG=671813
==========

to

==========
Remove some uses of isdigit in net/, as it can be locale depedent.

BUG=671813

Committed: https://crrev.com/0f8ee2261a801a18d55cab7f28d5551ca23ed012
Cr-Commit-Position: refs/heads/master@{#437045}
==========

[commit-bot: I haz the power, 2016-12-07 20:19:27]

Patchset 4 (id:??) landed as
https://crrev.com/0f8ee2261a801a18d55cab7f28d5551ca23ed012
Cr-Commit-Position: refs/heads/master@{#437045}