initialize result variables in v8 bindings

initialize result variables in v8 bindings

https://codereview.chromium.org/360703003 and
https://codereview.chromium.org/362993004 can falsely trigger
use of uninitialized variable warnings in some toolchains,
for integer "return" variables.  Let's explicitly initialize
these variables.

BUG=341031
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178263

[Mostyn Bramley-Moore, 2014-07-14 23:40:40]

@haraken: PTAL at this small change.

[haraken, 2014-07-15 00:58:02]

Does this apply to only int variables? What about unsigned, double etc?

[Mostyn Bramley-Moore, 2014-07-15 06:02:50]

On 2014/07/15 00:58:02, haraken wrote:
> Does this apply to only int variables? What about unsigned, double etc?

The other types don't seem to trigger the warning, but I'm not sure why.  Would
you like me to also initialize the other types for completeness?

[Jens Widell, 2014-07-15 14:31:56]

On 2014/07/15 06:02:50, Mostyn Bramley-Moore wrote:
> On 2014/07/15 00:58:02, haraken wrote:
> > Does this apply to only int variables? What about unsigned, double etc?
> 
> The other types don't seem to trigger the warning, but I'm not sure why. 
Would
> you like me to also initialize the other types for completeness?

FYI, https://codereview.chromium.org/337343002/ has a somewhat more ambitious
initializer implementation (among all the other stuff it does.)

Don't know if it makes sense to apply it here, but that's mostly because I don't
quite understand why the compiler thinks anything is uninitialized at all here.

[haraken, 2014-07-15 15:13:25]

I'm sorry for the late reply... missed the email.

https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:250: {{method.cpp_type}} result{% if
method.cpp_type == 'int' %} = 0{% endif %};

method.cpp_type == 'int' is a bit too hacky.

Can we add method.cpp_type_default_value and use it here? I think you can set
method.cpp_type_default_value to 0 if idl_type.is_numeric_type() is true.

[Jens Widell, 2014-07-15 15:50:29]

On 2014/07/15 15:13:25, haraken wrote:
> I'm sorry for the late reply... missed the email.
> 
>
https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
> Source/bindings/templates/methods.cpp:250: {{method.cpp_type}} result{% if
> method.cpp_type == 'int' %} = 0{% endif %};
> 
> method.cpp_type == 'int' is a bit too hacky.

Agree. That kind of code would just make the next person reading go "wtf?". :-)

> Can we add method.cpp_type_default_value and use it here? I think you can set
> method.cpp_type_default_value to 0 if idl_type.is_numeric_type() is true.

Using is_numeric_type doesn't cover boolean, which ought to need initialization
too if int does.

[haraken, 2014-07-15 15:52:44]

On 2014/07/15 15:50:29, Jens Lindström wrote:
> On 2014/07/15 15:13:25, haraken wrote:
> > I'm sorry for the late reply... missed the email.
> > 
> >
>
https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
> > File Source/bindings/templates/methods.cpp (right):
> > 
> >
>
https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
> > Source/bindings/templates/methods.cpp:250: {{method.cpp_type}} result{% if
> > method.cpp_type == 'int' %} = 0{% endif %};
> > 
> > method.cpp_type == 'int' is a bit too hacky.
> 
> Agree. That kind of code would just make the next person reading go "wtf?".
:-)
> 
> > Can we add method.cpp_type_default_value and use it here? I think you can
set
> > method.cpp_type_default_value to 0 if idl_type.is_numeric_type() is true.
> 
> Using is_numeric_type doesn't cover boolean, which ought to need
initialization
> too if int does.

Probably we can set method.cpp_type_default_value to 0 if is_numeric_type is
true, and set method.cpp_type_default_value to false if idl_type is boolean?

[Jens Widell, 2014-07-15 15:57:16]

On 2014/07/15 15:52:44, haraken wrote:
> On 2014/07/15 15:50:29, Jens Lindström wrote:
> > On 2014/07/15 15:13:25, haraken wrote:
> > > I'm sorry for the late reply... missed the email.
> > > 
> > >
> >
>
https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
> > > File Source/bindings/templates/methods.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/390223004/diff/1/Source/bindings/templates/me...
> > > Source/bindings/templates/methods.cpp:250: {{method.cpp_type}} result{% if
> > > method.cpp_type == 'int' %} = 0{% endif %};
> > > 
> > > method.cpp_type == 'int' is a bit too hacky.
> > 
> > Agree. That kind of code would just make the next person reading go "wtf?".
> :-)
> > 
> > > Can we add method.cpp_type_default_value and use it here? I think you can
> set
> > > method.cpp_type_default_value to 0 if idl_type.is_numeric_type() is true.
> > 
> > Using is_numeric_type doesn't cover boolean, which ought to need
> initialization
> > too if int does.
> 
> Probably we can set method.cpp_type_default_value to 0 if is_numeric_type is
> true, and set method.cpp_type_default_value to false if idl_type is boolean?

Yeah, that would certainly work. I'd prefer to call it "initializer" rather than
"default_value" though; "default_value" implies more that this is a value we
expect to use, and it isn't. (And if we were to do something equivalent for the
locals used for method arguments, then "default_value" would be outright
misleading.)

[haraken, 2014-07-15 15:59:09]

> > > > Can we add method.cpp_type_default_value and use it here? I think you
can
> > set
> > > > method.cpp_type_default_value to 0 if idl_type.is_numeric_type() is
true.
> > > 
> > > Using is_numeric_type doesn't cover boolean, which ought to need
> > initialization
> > > too if int does.
> > 
> > Probably we can set method.cpp_type_default_value to 0 if is_numeric_type is
> > true, and set method.cpp_type_default_value to false if idl_type is boolean?
> 
> Yeah, that would certainly work. I'd prefer to call it "initializer" rather
than
> "default_value" though; "default_value" implies more that this is a value we
> expect to use, and it isn't. (And if we were to do something equivalent for
the
> locals used for method arguments, then "default_value" would be outright
> misleading.)

Agreed.

[Mostyn Bramley-Moore, 2014-07-15 16:08:09]

OK, I will give these suggestions a try a bit later this evening.

[Mostyn Bramley-Moore, 2014-07-15 22:47:40]

Patchset 2 should be a bit more understandable.

[haraken, 2014-07-16 00:54:02]

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:133: cpp_type in ('float', 'double') or

Can't we use idl_type.is_numeric_type() ?

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:134: cpp_type.endswith('*')):

Can this happen?

Actually we don't want to mix idl_type and cpp_type to judge the types. I think
this function should be written based on idl_type only.

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:136: if cpp_type == 'bool':

idl_type.base_type == 'boolean'

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:139: return ' = ""'

We shouldn't add this. A String should be initialized with a null string instead
of an empty string.

[Jens Widell, 2014-07-16 07:00:54]

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:134: cpp_type.endswith('*')):
On 2014/07/16 00:54:02, haraken wrote:
> 
> Can this happen?
> 
> Actually we don't want to mix idl_type and cpp_type to judge the types. I
think
> this function should be written based on idl_type only.

This is copied from my patch (which is unpolished) where the local is sometimes
of pointer type. Shouldn't be needed for this CL, and should probably be done
differently (based only on idl_type, as you say).

[Mostyn Bramley-Moore, 2014-07-16 07:15:20]

Patchset 3 has the suggested refinements.

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:133: cpp_type in ('float', 'double') or
On 2014/07/16 00:54:02, haraken wrote:
> 
> Can't we use idl_type.is_numeric_type() ?

Done.

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:134: cpp_type.endswith('*')):
On 2014/07/16 07:00:53, Jens Lindström wrote:
> On 2014/07/16 00:54:02, haraken wrote:
> > 
> > Can this happen?
> > 
> > Actually we don't want to mix idl_type and cpp_type to judge the types. I
> think
> > this function should be written based on idl_type only.
> 
> This is copied from my patch (which is unpolished) where the local is
sometimes
> of pointer type. Shouldn't be needed for this CL, and should probably be done
> differently (based only on idl_type, as you say).

Done.

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:136: if cpp_type == 'bool':
On 2014/07/16 00:54:02, haraken wrote:
> 
> idl_type.base_type == 'boolean'

Done.

https://codereview.chromium.org/390223004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:139: return ' = ""'
On 2014/07/16 00:54:02, haraken wrote:
> 
> We shouldn't add this. A String should be initialized with a null string
instead
> of an empty string.

Done.

[Jens Widell, 2014-07-16 07:25:10]

https://codereview.chromium.org/390223004/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/390223004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:134: return ''
Return None by default?

https://codereview.chromium.org/390223004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:135: 
Two blank lines between function definitions.

I'd put this after cpp_type(). Also consider putting it as a property on IdlType
(like cpp_type) to make it accessible simply as

  idl_type.cpp_type_initializer

elsewhere.

[Mostyn Bramley-Moore, 2014-07-16 07:46:56]

https://codereview.chromium.org/390223004/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/390223004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:134: return ''
On 2014/07/16 07:25:10, Jens Lindström wrote:
> Return None by default?

If I do that then we can't blindly use {{attribute.cpp_type_initializer}} in the
templates- it would have to be wrapped in a check that it's not None.

https://codereview.chromium.org/390223004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:135: 
On 2014/07/16 07:25:10, Jens Lindström wrote:
> Two blank lines between function definitions.
> 
> I'd put this after cpp_type(). Also consider putting it as a property on
IdlType
> (like cpp_type) to make it accessible simply as
> 
>   idl_type.cpp_type_initializer
> 
> elsewhere.

Done.

[Jens Widell, 2014-07-16 07:52:34]

LGTM. (Non-owner.)

[Mostyn Bramley-Moore, 2014-07-16 08:04:01]

The CQ bit was checked by mostynb@opera.com

[Mostyn Bramley-Moore, 2014-07-16 08:04:03]

The CQ bit was unchecked by mostynb@opera.com

[haraken, 2014-07-16 08:15:02]

LGTM

[Mostyn Bramley-Moore, 2014-07-16 08:16:25]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-07-16 08:17:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mostynb@opera.com/390223004/60001

[commit-bot: I haz the power, 2014-07-16 09:49:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17211)

[commit-bot: I haz the power, 2014-07-16 10:48:54]

Change committed as 178263