Address compilation warnings for android build.

Address compilation warnings for android build.

LOG=N
BUG=

Committed: https://crrev.com/b215c9e5b837210fb5839ecf3ee2d1a9e771d591
Cr-Commit-Position: refs/heads/master@{#37676}

[oth, 2016-07-08 10:01:15]

oth@chromium.org changed reviewers:
+ adamk@chromium.org, bmeurer@chromium.org

[oth, 2016-07-08 10:01:16]

This CL addresses some trivial warnings emitted during the android build, e.g.
signed vs unsigned char, inappropriate range changes, etc.

bmeurer@chromium.org: Please review changes in src/uri.h
compiler/simplified-lowering.cc

adamk@chromium.org: Please review changes in parsing/scanner.h

rmcilroy@chromium.org: Please review changes in interpreter/ and cctest/

[rmcilroy, 2016-07-08 11:15:05]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2016-07-08 11:15:06]

interpreter LGTM

https://codereview.chromium.org/2135573002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2135573002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:637: DCHECK_LE(start_index, start_index +
size);
Are we worried about overflow here? I'd just remove the second DCHECK.

https://codereview.chromium.org/2135573002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:658: DCHECK_LE(first_yield, first_yield +
stmt->yield_count());
ditto

https://codereview.chromium.org/2135573002/diff/1/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2135573002/diff/1/src/parsing/scanner.h#newco...
src/parsing/scanner.h:165: //    DCHECK((0 < code_unit && code_unit <=
kMaxAscii) || code_unit == 0);
Remove comment?

[Benedikt Meurer, 2016-07-08 18:34:26]

LGTM on uri and compiler.

[oth, 2016-07-10 15:34:16]

oth@chromium.org changed reviewers:
+ vogelheim@chromium.org
- adamk@chromium.org

[oth, 2016-07-10 15:34:16]

-adamk [OOO]
+vogelheim

Daniel, could you take a look at scanner.h? Trivial fix for flood of warnings
during android build?

Thanks

https://codereview.chromium.org/2135573002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2135573002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:637: DCHECK_LE(start_index, start_index +
size);
On 2016/07/08 11:15:06, rmcilroy wrote:
> Are we worried about overflow here? I'd just remove the second DCHECK.

Done. It matched the semantic of the original checks.

https://codereview.chromium.org/2135573002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:658: DCHECK_LE(first_yield, first_yield +
stmt->yield_count());
On 2016/07/08 11:15:06, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/2135573002/diff/1/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2135573002/diff/1/src/parsing/scanner.h#newco...
src/parsing/scanner.h:165: //    DCHECK((0 < code_unit && code_unit <=
kMaxAscii) || code_unit == 0);
On 2016/07/08 11:15:06, rmcilroy wrote:
> Remove comment?

Done.

[vogelheim, 2016-07-11 07:42:49]

https://codereview.chromium.org/2135573002/diff/20001/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2135573002/diff/20001/src/parsing/scanner.h#n...
src/parsing/scanner.h:165: DCHECK(iscntrl(code_unit) || isprint(code_unit));
Hrm. I searched for several minutes until I had found suitable defintions for
iscntrl and isprint, so I could determine whether this is correct. The good news
is, of course it's correct.

The bad news is, it took me quite a bit to figure that out.

Can we have a warning-free version that just checks upper & lower bounds?

[oth, 2016-07-11 08:43:30]

On 2016/07/11 07:42:49, vogelheim wrote:
> https://codereview.chromium.org/2135573002/diff/20001/src/parsing/scanner.h
> File src/parsing/scanner.h (right):
> 
>
https://codereview.chromium.org/2135573002/diff/20001/src/parsing/scanner.h#n...
> src/parsing/scanner.h:165: DCHECK(iscntrl(code_unit) || isprint(code_unit));
> Hrm. I searched for several minutes until I had found suitable defintions for
> iscntrl and isprint, so I could determine whether this is correct. The good
news
> is, of course it's correct.
> 
> The bad news is, it took me quite a bit to figure that out.
> 
> Can we have a warning-free version that just checks upper & lower bounds?

It may be possible, but the offending compiler seems pretty good at spotting the
always true case when chars are unsigned. I'll take another look.

Other options might be:

o have an IsValid(char) method that has an ifdef'ed validation method for signed
vs unsigned char.
o adding a comment about the ranges checked by the two statements help?

Thanks
Orion

[oth, 2016-07-12 08:28:00]

On 2016/07/11 08:43:30, oth wrote:
> On 2016/07/11 07:42:49, vogelheim wrote:
> > https://codereview.chromium.org/2135573002/diff/20001/src/parsing/scanner.h
> > File src/parsing/scanner.h (right):
> > 
> >
>
https://codereview.chromium.org/2135573002/diff/20001/src/parsing/scanner.h#n...
> > src/parsing/scanner.h:165: DCHECK(iscntrl(code_unit) || isprint(code_unit));
> > Hrm. I searched for several minutes until I had found suitable defintions
for
> > iscntrl and isprint, so I could determine whether this is correct. The good
> news
> > is, of course it's correct.
> > 
> > The bad news is, it took me quite a bit to figure that out.
> > 
> > Can we have a warning-free version that just checks upper & lower bounds?
> 
> It may be possible, but the offending compiler seems pretty good at spotting
the
> always true case when chars are unsigned. I'll take another look.
> 
> Other options might be:
> 
> o have an IsValid(char) method that has an ifdef'ed validation method for
signed
> vs unsigned char.
> o adding a comment about the ranges checked by the two statements help?
> 
> Thanks
> Orion

Daniel, the code now has an IsValidAscii() method together with a comment
describing the check. WDYT?

Thanks

[vogelheim, 2016-07-12 11:01:41]

lgtm

[oth, 2016-07-12 11:19:08]

The CQ bit was checked by oth@chromium.org

[oth, 2016-07-12 11:19:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2135573002/#ps60001
(title: "Update comment.")

[commit-bot: I haz the power, 2016-07-12 11:19:21]

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-07-12 11:21:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-12 11:22:04]

Description was changed from

==========
Address compilation warnings for android build.

LOG=N
BUG=
==========

to

==========
Address compilation warnings for android build.

LOG=N
BUG=

Committed: https://crrev.com/b215c9e5b837210fb5839ecf3ee2d1a9e771d591
Cr-Commit-Position: refs/heads/master@{#37676}
==========

[commit-bot: I haz the power, 2016-07-12 11:22:05]

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