Pass debug name as Vector instead of const char*

Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, and values containing null
characters. Both might happen in wasm.

R=titzer@chromium.org

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

[Clemens Hammacher, 2016-04-22 11:02:58]

Description was changed from

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, like wasm function
names. The alternative would be to copy them and add a trailing '\0',
which adds runtime overhead.
This prepares a commit to show wasm function names: <LINK HERE>

R=titzer@chromium.org
==========

to

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, like wasm function
names. The alternative would be to copy them and add a trailing '\0',
which adds runtime overhead.
This prepares a commit to show wasm function names: <LINK HERE>

R=titzer@chromium.org, mstarzinger@chromium.org
==========

[Clemens Hammacher, 2016-04-22 11:02:58]

clemensh@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Clemens Hammacher, 2016-04-22 11:04:12]

Description was changed from

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, like wasm function
names. The alternative would be to copy them and add a trailing '\0',
which adds runtime overhead.
This prepares a commit to show wasm function names: <LINK HERE>

R=titzer@chromium.org, mstarzinger@chromium.org
==========

to

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, like wasm function
names. The alternative would be to copy them and add a trailing '\0',
which adds runtime overhead.
This prepares a commit to show wasm function names: <LINK HERE>

R=titzer@chromium.org
==========

[Clemens Hammacher, 2016-04-22 11:04:12]

clemensh@chromium.org changed reviewers:
- mstarzinger@chromium.org

[titzer, 2016-04-22 11:46:30]

https://codereview.chromium.org/1911313002/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1911313002/diff/1/src/compiler.h#newcode465
src/compiler.h:465: Vector<const char> GetFunctionName() const { return
function_name_; }
If this is only called internally, I would recommend dropping it.

https://codereview.chromium.org/1911313002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1911313002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2955: ->GetNameOrNull(function.name_offset,
function.name_length)
You should probably just make this method return Vector<const char>.

https://codereview.chromium.org/1911313002/diff/1/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/1911313002/diff/1/src/vector.h#newcode207
src/vector.h:207: template <typename T, int N>
Since this is only ever used for C strings (now), I'd suggest:

inline Vector<const char> CStrVector(const char*[N])

https://codereview.chromium.org/1911313002/diff/1/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/1911313002/diff/1/src/wasm/wasm-opcodes.h#new...
src/wasm/wasm-opcodes.h:55: // TODO(clemensh): Remove whole WasmName, replace by
Vector<const char>
This should be inline, to prevent a proliferation of itty bitty helper
functions.

[Clemens Hammacher, 2016-04-22 13:47:54]

https://codereview.chromium.org/1911313002/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1911313002/diff/1/src/compiler.h#newcode465
src/compiler.h:465: Vector<const char> GetFunctionName() const { return
function_name_; }
On 2016/04/22 at 11:46:30, titzer wrote:
> If this is only called internally, I would recommend dropping it.

OK, dropped it.

https://codereview.chromium.org/1911313002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1911313002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2955: ->GetNameOrNull(function.name_offset,
function.name_length)
On 2016/04/22 at 11:46:30, titzer wrote:
> You should probably just make this method return Vector<const char>.

Done in https://codereview.chromium.org/1910213004

https://codereview.chromium.org/1911313002/diff/1/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/1911313002/diff/1/src/vector.h#newcode207
src/vector.h:207: template <typename T, int N>
On 2016/04/22 at 11:46:30, titzer wrote:
> Since this is only ever used for C strings (now), I'd suggest:
> 
> inline Vector<const char> CStrVector(const char*[N])

CStrVector is already there, but it takes a const char*, and computes the string
length itself; this is unnecessary in many cases.

ArrayVector was meant to be a type-safe generalization of STATIC_CHAR_VECTOR,
and there are indeed 28 places where a pattern like 'Vector<T> vec(buffer,
arraysize(buffer))' is used, where T is char, const char* or HValue*.
Is it desirable to refactor such code, in the belief that ArrayVector is much
nicer?

https://codereview.chromium.org/1911313002/diff/1/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/1911313002/diff/1/src/wasm/wasm-opcodes.h#new...
src/wasm/wasm-opcodes.h:55: // TODO(clemensh): Remove whole WasmName, replace by
Vector<const char>
On 2016/04/22 at 11:46:30, titzer wrote:
> This should be inline, to prevent a proliferation of itty bitty helper
functions.

I will remove the whole WasmName in https://codereview.chromium.org/1910213004.

[titzer, 2016-04-22 13:51:45]

https://codereview.chromium.org/1911313002/diff/1/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/1911313002/diff/1/src/vector.h#newcode207
src/vector.h:207: template <typename T, int N>
On 2016/04/22 13:47:54, Clemens Hammacher wrote:
> On 2016/04/22 at 11:46:30, titzer wrote:
> > Since this is only ever used for C strings (now), I'd suggest:
> > 
> > inline Vector<const char> CStrVector(const char*[N])
> 
> CStrVector is already there, but it takes a const char*, and computes the
string
> length itself; this is unnecessary in many cases.
> 
> ArrayVector was meant to be a type-safe generalization of STATIC_CHAR_VECTOR,
> and there are indeed 28 places where a pattern like 'Vector<T> vec(buffer,
> arraysize(buffer))' is used, where T is char, const char* or HValue*.
> Is it desirable to refactor such code, in the belief that ArrayVector is much
> nicer?

Yeah, I'd rather just make a small addition now and fork off refactoring of the
other places in V8 in another CL. So you can just add a variant of CStrVector if
its possible. I find it reads nicer at call sites to see
CStrVector("somestring") instead of ArrayVector("somestring"): again,
communicating intent.

[titzer, 2016-04-22 13:54:22]

On 2016/04/22 13:51:45, titzer wrote:
> https://codereview.chromium.org/1911313002/diff/1/src/vector.h
> File src/vector.h (right):
> 
> https://codereview.chromium.org/1911313002/diff/1/src/vector.h#newcode207
> src/vector.h:207: template <typename T, int N>
> On 2016/04/22 13:47:54, Clemens Hammacher wrote:
> > On 2016/04/22 at 11:46:30, titzer wrote:
> > > Since this is only ever used for C strings (now), I'd suggest:
> > > 
> > > inline Vector<const char> CStrVector(const char*[N])
> > 
> > CStrVector is already there, but it takes a const char*, and computes the
> string
> > length itself; this is unnecessary in many cases.
> > 
> > ArrayVector was meant to be a type-safe generalization of
STATIC_CHAR_VECTOR,
> > and there are indeed 28 places where a pattern like 'Vector<T> vec(buffer,
> > arraysize(buffer))' is used, where T is char, const char* or HValue*.
> > Is it desirable to refactor such code, in the belief that ArrayVector is
much
> > nicer?
> 
> Yeah, I'd rather just make a small addition now and fork off refactoring of
the
> other places in V8 in another CL. So you can just add a variant of CStrVector
if
> its possible. I find it reads nicer at call sites to see
> CStrVector("somestring") instead of ArrayVector("somestring"): again,
> communicating intent.

After all, I don't think we want to invest too much more into V8's kind of wimpy
Vector, since we are starting to spread std::vector with abandon.

[Clemens Hammacher, 2016-04-26 09:57:11]

Description was changed from

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, like wasm function
names. The alternative would be to copy them and add a trailing '\0',
which adds runtime overhead.
This prepares a commit to show wasm function names: <LINK HERE>

R=titzer@chromium.org
==========

to

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, like wasm function
names. The alternative would be to copy them and add a trailing '\0',
which adds runtime overhead.

R=titzer@chromium.org
==========

[Clemens Hammacher, 2016-04-26 10:00:13]

Description was changed from

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, like wasm function
names. The alternative would be to copy them and add a trailing '\0',
which adds runtime overhead.

R=titzer@chromium.org
==========

to

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, and values containing null
characters. Both might happen in wasm.

R=titzer@chromium.org
==========

[Clemens Hammacher, 2016-04-26 12:36:47]

As discussed offline, I stick to ArrayVector, since another variant of
CStrVector causes errors in other places, where null-terminated c-strings in
bigger arrays are not interpreted differently.

PTAL

[titzer, 2016-04-26 12:45:58]

https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h...
src/wasm/wasm-opcodes.h:56: inline Vector<const char> toVec() const {
We don't need this little helper function. Either inline it into the caller or
let's typedef Vector<const char> to WasmName.

[Clemens Hammacher, 2016-04-26 12:58:53]

https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h...
src/wasm/wasm-opcodes.h:56: inline Vector<const char> toVec() const {
On 2016/04/26 at 12:45:57, titzer wrote:
> We don't need this little helper function. Either inline it into the caller or
let's typedef Vector<const char> to WasmName.

Sorry, forgot to mention that. The TODO will be handled in this CL:
https://codereview.chromium.org/1910213004

[Clemens Hammacher, 2016-04-26 14:28:43]

On 2016/04/26 at 12:58:53, Clemens Hammacher wrote:
> https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h
> File src/wasm/wasm-opcodes.h (right):
> 
>
https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h...
> src/wasm/wasm-opcodes.h:56: inline Vector<const char> toVec() const {
> On 2016/04/26 at 12:45:57, titzer wrote:
> > We don't need this little helper function. Either inline it into the caller
or let's typedef Vector<const char> to WasmName.
> 
> Sorry, forgot to mention that. The TODO will be handled in this CL:
https://codereview.chromium.org/1910213004

Is that a lgtm otherwise?

[titzer, 2016-04-26 14:32:51]

On 2016/04/26 14:28:43, Clemens Hammacher wrote:
> On 2016/04/26 at 12:58:53, Clemens Hammacher wrote:
> >
https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h
> > File src/wasm/wasm-opcodes.h (right):
> > 
> >
>
https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h...
> > src/wasm/wasm-opcodes.h:56: inline Vector<const char> toVec() const {
> > On 2016/04/26 at 12:45:57, titzer wrote:
> > > We don't need this little helper function. Either inline it into the
caller
> or let's typedef Vector<const char> to WasmName.
> > 
> > Sorry, forgot to mention that. The TODO will be handled in this CL:
> https://codereview.chromium.org/1910213004
> 
> Is that a lgtm otherwise?

Yes.

[titzer, 2016-04-26 14:33:25]

On 2016/04/26 14:32:51, titzer wrote:
> On 2016/04/26 14:28:43, Clemens Hammacher wrote:
> > On 2016/04/26 at 12:58:53, Clemens Hammacher wrote:
> > >
> https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h
> > > File src/wasm/wasm-opcodes.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1911313002/diff/40001/src/wasm/wasm-opcodes.h...
> > > src/wasm/wasm-opcodes.h:56: inline Vector<const char> toVec() const {
> > > On 2016/04/26 at 12:45:57, titzer wrote:
> > > > We don't need this little helper function. Either inline it into the
> caller
> > or let's typedef Vector<const char> to WasmName.
> > > 
> > > Sorry, forgot to mention that. The TODO will be handled in this CL:
> > https://codereview.chromium.org/1910213004
> > 
> > Is that a lgtm otherwise?
> 
> Yes.

lgtm

[Clemens Hammacher, 2016-04-26 14:34:11]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2016-04-26 14:34:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1911313002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1911313002/40001

[commit-bot: I haz the power, 2016-04-26 14:36:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-26 14:37:16]

Description was changed from

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, and values containing null
characters. Both might happen in wasm.

R=titzer@chromium.org
==========

to

==========
Pass debug name as Vector instead of const char*

This allows to also pass non-null-terminated values, and values containing null
characters. Both might happen in wasm.

R=titzer@chromium.org

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

[commit-bot: I haz the power, 2016-04-26 14:37:17]

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