V8. ASM-2-WASM. Another asm-types.h revision.

V8. ASM-2-WASM. Another asm-types.h revision.

BUG=

Committed: https://crrev.com/4257fdea1e63fe226c0cb192d097c2c8c08db9df
Cr-Commit-Position: refs/heads/master@{#37110}

[John, 2016-06-17 22:27:39]

jpp@chromium.org changed reviewers:
+ bradnelson@chromium.org

[John, 2016-06-17 22:27:40]

Seems like this is pretty close to what the validator needs.

[bradnelson, 2016-06-17 23:15:33]

Merge woes?

https://codereview.chromium.org/2078053002/diff/40001/src/wasm/asm-types.h
File src/wasm/asm-types.h (right):

https://codereview.chromium.org/2078053002/diff/40001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:51: /* Pseudo-types used in representing heap access for fp
types.*/            \
What happened to 22?

[John, 2016-06-17 23:19:35]

Patchset #4 (id:60001) has been deleted

[John, 2016-06-17 23:20:24]

maybe ready for another pass...

[John, 2016-06-17 23:23:03]

https://codereview.chromium.org/2078053002/diff/40001/src/wasm/asm-types.h
File src/wasm/asm-types.h (right):

https://codereview.chromium.org/2078053002/diff/40001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:51: /* Pseudo-types used in representing heap access for fp
types.*/            \
On 2016/06/17 23:15:33, bradnelson wrote:
> What happened to 22?

Done.

[bradnelson, 2016-06-17 23:38:50]

lgtm, but can you update the commit message with this:

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=unittests/asm-types-unittest
R=bradnelson@chromium.org
LOG=N

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.cc
File src/wasm/asm-types.cc (right):

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:329: if (signature_ == nullptr) {
Doesn't this need to check that function_type is a function before adding it in?
(Or are you doing that outside for a better error message, yeah do that).

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.h
File src/wasm/asm-types.h (right):

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:194: void set_signature(AsmType* function_type);
Will you need a method to check if the signature was ever set?

https://codereview.chromium.org/2078053002/diff/80001/test/unittests/wasm/asm...
File test/unittests/wasm/asm-types-unittest.cc (right):

https://codereview.chromium.org/2078053002/diff/80001/test/unittests/wasm/asm...
test/unittests/wasm/asm-types-unittest.cc:384: // Call with extern type should
fail.
Comment seems off here

[John, 2016-06-20 15:09:53]

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.cc
File src/wasm/asm-types.cc (right):

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:329: if (signature_ == nullptr) {
On 2016/06/17 23:38:50, bradnelson wrote:
> Doesn't this need to check that function_type is a function before adding it
in?
> (Or are you doing that outside for a better error message, yeah do that).

signature_ is now initialized during the constructor.

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.h
File src/wasm/asm-types.h (right):

https://codereview.chromium.org/2078053002/diff/80001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:194: void set_signature(AsmType* function_type);
On 2016/06/17 23:38:50, bradnelson wrote:
> Will you need a method to check if the signature was ever set?

Not really.

tl;dr Looking at how this class is used in typing-asm (not on this CL) this
method is not really necessary. Jump table signatures are known when the types
are created, so I'll remove it.

https://codereview.chromium.org/2078053002/diff/80001/test/unittests/wasm/asm...
File test/unittests/wasm/asm-types-unittest.cc (right):

https://codereview.chromium.org/2078053002/diff/80001/test/unittests/wasm/asm...
test/unittests/wasm/asm-types-unittest.cc:384: // Call with extern type should
fail.
On 2016/06/17 23:38:50, bradnelson wrote:
> Comment seems off here

Done.

[John, 2016-06-20 16:33:48]

The CQ bit was checked by jpp@chromium.org

[John, 2016-06-20 16:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2078053002/#ps140001
(title: "git pull")

[commit-bot: I haz the power, 2016-06-20 16:33:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078053002/140001

[commit-bot: I haz the power, 2016-06-20 16:53:27]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-06-20 16:54:54]

Description was changed from

==========
V8. ASM-2-WASM. Another asm-types.h revision.

BUG=
==========

to

==========
V8. ASM-2-WASM. Another asm-types.h revision.

BUG=

Committed: https://crrev.com/4257fdea1e63fe226c0cb192d097c2c8c08db9df
Cr-Commit-Position: refs/heads/master@{#37110}
==========

[commit-bot: I haz the power, 2016-06-20 16:54:55]

Patchset 7 (id:??) landed as
https://crrev.com/4257fdea1e63fe226c0cb192d097c2c8c08db9df
Cr-Commit-Position: refs/heads/master@{#37110}

[Mostyn Bramley-Moore, 2016-07-04 20:23:35]

mostynb@opera.com changed reviewers:
+ mostynb@opera.com

[Mostyn Bramley-Moore, 2016-07-04 20:23:36]

https://codereview.chromium.org/2078053002/diff/140001/src/wasm/asm-types.cc
File src/wasm/asm-types.cc (right):

https://codereview.chromium.org/2078053002/diff/140001/src/wasm/asm-types.cc#...
src/wasm/asm-types.cc:309: return signature_->Name() + "[" +
std::to_string(length_) + "]";
std::to_string is in the "C++11 Standard Library Features To Be Discussed"
section of the C++11 use in Chromium doc:
https://chromium-cpp.appspot.com/#library-review

Should this be discussed at cxx@chromium.org first?