Issue 2487673004: [wasm] Fix -Wsign-compare warnings.

Issue 2487673004: [wasm] Fix -Wsign-compare warnings. (Closed)

Created:
4 years, 1 month ago by ulan

Modified:
4 years, 1 month ago

Reviewers:
ahaas

CC:
v8-reviews_googlegroups.com

Target Ref:
refs/pending/heads/master

Project:
v8

Visibility:
Public.

More Reviews

Description

[wasm] Fix -Wsign-compare warnings. BUG=v8:5614 Committed: https://crrev.com/3505406bc7493795de3471acf0c90c187adcc241 Cr-Commit-Position: refs/heads/master@{#40891}

Patch Set 2 : address comments #

Created: 4 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+57 lines, -53 lines)			Patch
M	src/asmjs/asm-typer.cc	View		4 chunks	+4 lines, -4 lines	0 comments	Download
M	src/wasm/ast-decoder.cc	View		5 chunks	+6 lines, -6 lines	0 comments	Download
M	src/wasm/wasm-module.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	test/cctest/asmjs/test-asm-typer.cc	View		2 chunks	+4 lines, -2 lines	0 comments	Download
M	test/cctest/test-run-wasm-relocation-ia32.cc	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	test/cctest/test-run-wasm-relocation-x64.cc	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	test/cctest/wasm/test-run-wasm.cc	View	1	4 chunks	+4 lines, -4 lines	0 comments	Download
M	test/cctest/wasm/test-run-wasm-64.cc	View	1	8 chunks	+16 lines, -16 lines	0 comments	Download
M	test/cctest/wasm/test-run-wasm-interpreter.cc	View		2 chunks	+4 lines, -2 lines	0 comments	Download
M	test/fuzzer/fuzzer.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	test/fuzzer/wasm-call.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	test/fuzzer/wasm-code.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	test/unittests/wasm/ast-decoder-unittest.cc	View		5 chunks	+5 lines, -5 lines	0 comments	Download
M	test/unittests/wasm/leb-helper-unittest.cc	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	test/unittests/wasm/module-decoder-unittest.cc	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 10 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

ahaas

lgtm with nits. Thanks for this work! https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-wasm-64.cc File test/cctest/wasm/test-run-wasm-64.cc (right): https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-wasm-64.cc#newcode201 test/cctest/wasm/test-run-wasm-64.cc:201: uint64_t expected ...

4 years, 1 month ago (2016-11-10 09:03:39 UTC) #3

ulan

Thanks! https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-wasm-64.cc File test/cctest/wasm/test-run-wasm-64.cc (right): https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-wasm-64.cc#newcode201 test/cctest/wasm/test-run-wasm-64.cc:201: uint64_t expected = static_cast<int32_t>((*i) << (*j & 0x3f)); ...

4 years, 1 month ago (2016-11-10 12:23:43 UTC) #4

Thanks!

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
File test/cctest/wasm/test-run-wasm-64.cc (right):

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:201: uint64_t expected =
static_cast<int32_t>((*i) << (*j & 0x3f));
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change the type of {expected} to int32_t instead of doing the
> static_cast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:216: uint64_t expected =
static_cast<int32_t>((*i) >> (*j & 0x3f));
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change the type of {expected} to int32_t instead of doing the
> static_cast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:231: uint64_t expected =
static_cast<int32_t>((*i) >> (*j & 0x3f));
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change the type of {expected} to int32_t instead of doing the
> static_cast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:425: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:430: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:435: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:440: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:461: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:466: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:473: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:480: WasmRunner<int64_t> r(execution_mode,
MachineType::Int64());
On 2016/11/10 09:03:39, ahaas wrote:
> Can you change
> WasmRunner<int64_t> r(execution_mode, MachineType::Int64());
> to
> WasmRunner<uint64_t> r(execution_mode, MachineType::Uint64());
> here instead of the bitcast below?

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:1375:
module.WriteMemory<int64_t>(&memory[0], 0xaabbccdd00112233LL);
On 2016/11/10 09:03:39, ahaas wrote:
> I think you could just change 0xaabbccdd00112233LL to 0x1abbccdd00112233LL to
> avoid the sign bit.

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-64.cc:1396:
module.WriteMemory<int64_t>(&memory[0], 0xaabbccdd00112233LL);
On 2016/11/10 09:03:39, ahaas wrote:
> same here.

Done.

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2487673004/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm.cc:1210: WasmRunner<int32_t> r(execution_mode,
MachineType::Uint32(),
On 2016/11/10 09:03:39, ahaas wrote:
> I think it would be better to not change the template of the WasmRunner, but
> instead change FOR_INT32_INPUTS(i) to FOR_UINT32_INPUTS(i). 

Done.

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-10 12:24:08 UTC) #7

commit-bot: I haz the power

4 years, 1 month ago (2016-11-17 22:29:08 UTC) #10

Message was sent while issue was closed.

Patchset 2 (id:??) landed as
https://crrev.com/3505406bc7493795de3471acf0c90c187adcc241
Cr-Commit-Position: refs/heads/master@{#40891}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages