Remove UnusedParam.h

Remove UnusedParam.h as most of its use is not not needed.

Chromium and Blink sets -Wno-unused-parameter intentionally, so no need to silence related warnings using the UNUSED_PARAM macro. 

Introduce ALLOW_UNUSED for Blink to mark local variables that might be unused to silence -Wunused-variable. The same macro exists on the chromium side with the same definition.

Replace UNUSED_PARAM with the newly introduced ALLOW_UNUSED where needed.

TBR=dglazkov@chromium.org

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

[lgombos, 2013-10-22 19:36:17]

This CL includes "UnusedParam.h" where UNUSED_PARAM is used (and removes it
where it does not). 

As I was working on this change, I realized that it might improve readability
and hackability if instead we would just make UNUSED_PARAM available globally in
config.h, similar to OS() and COMPILER() macros. 

Any thoughts on this ? Do people prefer land this CL or make UNUSED_PARAM
available across blink ?

[abarth-chromium, 2013-10-22 19:48:30]

I don't think we need UNUSED_PARAM anymore.  We don't have that warning turned
on.

[lgombos, 2013-10-23 20:46:39]

On 2013/10/22 19:48:30, abarth wrote:
> I don't think we need UNUSED_PARAM anymore.  We don't have that warning turned
> on.

That is indeed the case for blink. However for the rest of chromium (with some
exceptions for third_party modules) error=unused-variable is on.

Should we reconsider using error=unused-variable for blink with some limitation
(e.g. perhaps setting no-unused-parameter) to try to match rest of chromium
policy ?

[abarth-chromium, 2013-10-24 14:08:25]

On 2013/10/23 20:46:39, lgombos wrote:
> On 2013/10/22 19:48:30, abarth wrote:
> > I don't think we need UNUSED_PARAM anymore.  We don't have that warning
turned
> > on.
> 
> That is indeed the case for blink. However for the rest of chromium (with some
> exceptions for third_party modules) error=unused-variable is on.
> 
> Should we reconsider using error=unused-variable for blink with some
limitation
> (e.g. perhaps setting no-unused-parameter) to try to match rest of chromium
> policy ?

We should either turn this error on and have this macro or delete the macro. 
Having the error off and having the macro makes very little sense.

[Inactive, 2013-10-24 14:10:41]

On 2013/10/24 14:08:25, abarth wrote:
> On 2013/10/23 20:46:39, lgombos wrote:
> > On 2013/10/22 19:48:30, abarth wrote:
> > > I don't think we need UNUSED_PARAM anymore.  We don't have that warning
> turned
> > > on.
> > 
> > That is indeed the case for blink. However for the rest of chromium (with
some
> > exceptions for third_party modules) error=unused-variable is on.
> > 
> > Should we reconsider using error=unused-variable for blink with some
> limitation
> > (e.g. perhaps setting no-unused-parameter) to try to match rest of chromium
> > policy ?
> 
> We should either turn this error on and have this macro or delete the macro. 
> Having the error off and having the macro makes very little sense.

Considering chromium has this error enabled, I think it would make sense to
enable it for Blink as well.

[Inactive, 2013-10-24 14:12:24]

On 2013/10/24 14:10:41, Chris Dumez wrote:
> On 2013/10/24 14:08:25, abarth wrote:
> > On 2013/10/23 20:46:39, lgombos wrote:
> > > On 2013/10/22 19:48:30, abarth wrote:
> > > > I don't think we need UNUSED_PARAM anymore.  We don't have that warning
> > turned
> > > > on.
> > > 
> > > That is indeed the case for blink. However for the rest of chromium (with
> some
> > > exceptions for third_party modules) error=unused-variable is on.
> > > 
> > > Should we reconsider using error=unused-variable for blink with some
> > limitation
> > > (e.g. perhaps setting no-unused-parameter) to try to match rest of
chromium
> > > policy ?
> > 
> > We should either turn this error on and have this macro or delete the macro.

> > Having the error off and having the macro makes very little sense.
> 
> Considering chromium has this error enabled, I think it would make sense to
> enable it for Blink as well.

Is it worth asking on Blink-dev what people think maybe?

[abarth-chromium, 2013-10-24 14:38:08]

On 2013/10/24 14:12:24, Chris Dumez wrote:
> Is it worth asking on Blink-dev what people think maybe?

Sure, that sounds like a good thing to do.

[lgombos, 2013-12-02 04:37:41]

On 2013/10/24 14:38:08, abarth wrote:
> On 2013/10/24 14:12:24, Chris Dumez wrote:
> > Is it worth asking on Blink-dev what people think maybe?
> 
> Sure, that sounds like a good thing to do.

-Werror=unused-variable is turned on for chromium and (now that bug 312287 is
fixed) on blink as well. UNUSED_PARAM is used to silence this warning.

As an example removing UNUSED_PARAM from ThreadingPthreads.cpp
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
will cause a build failure (tried on Linux gcc).

Seems we need to keep UnusedParam.h around. I will resume this work.

[lgombos, 2013-12-02 06:21:00]

Rebased and putting it up for review again.

[Inactive, 2013-12-05 19:30:52]

On 2013/12/02 06:21:00, lgombos wrote:
> Rebased and putting it up for review again.

It seems odd to have a header only for this macro. How about moving it to
wtf/Compiler.h header? (that header is included from config.h so we wouldn't
need to include it everywhere). After all, the purpose of this macro is to
silent *compiler* warnings.

Note that there is already WARN_UNUSED_RETURN macro in wtf/Compiler.h which is a
similar macro IMHO.

abarth, eseidel, any opinion on this?

[Nils Barth (inactive), 2013-12-06 01:52:27]

On 2013/12/05 19:30:52, Chris Dumez wrote:
> On 2013/12/02 06:21:00, lgombos wrote:
> > Rebased and putting it up for review again.
> 
> It seems odd to have a header only for this macro.
> How about moving it to wtf/Compiler.h header?
> (that header is included from config.h so we wouldn't need to include it
everywhere).
> After all, the purpose of this macro is to silent *compiler* warnings.

FWIW: Sounds reasonable at a glance (both are just macros).

Quick note:
UnusedParam.h is included in all generated bindings;
would replacing this with Compiler.h cause any issues
(e.g., slowing down build)?
I assume not, since it's just a few macros, but wanted to note this.

[Inactive, 2013-12-06 02:25:41]

On 2013/12/06 01:52:27, Nils Barth wrote:
> On 2013/12/05 19:30:52, Chris Dumez wrote:
> > On 2013/12/02 06:21:00, lgombos wrote:
> > > Rebased and putting it up for review again.
> > 
> > It seems odd to have a header only for this macro.
> > How about moving it to wtf/Compiler.h header?
> > (that header is included from config.h so we wouldn't need to include it
> everywhere).
> > After all, the purpose of this macro is to silent *compiler* warnings.
> 
> FWIW: Sounds reasonable at a glance (both are just macros).
> 
> Quick note:
> UnusedParam.h is included in all generated bindings;
> would replacing this with Compiler.h cause any issues
> (e.g., slowing down build)?
> I assume not, since it's just a few macros, but wanted to note this.

As I said, wtf/Compiler.h is already included from config.h. So we wouldn't need
to explicitly include wtf/Compiler.h. We could simply get rid of the
UnusedParam.h includes. This applies to the bindings as well and they include
config.h.

[lgombos, 2013-12-06 19:35:01]

Nico, what do you think ? Should we continue to maintain a separate
UnusedParam.h or fold it into wtf/Compiler.h (or perhaps somewhere else) ?

[Nico, 2013-12-06 19:40:51]

I thought this macro was for -Wunused-param (which we don't use) not
-Wunused-variable?

[lgombos, 2013-12-06 20:10:00]

On 2013/12/06 19:40:51, Nico wrote:
> I thought this macro was for -Wunused-param (which we don't use) not
> -Wunused-variable?

We also (mis)use it for -Werror=unused-variable - see my previous comment -
https://codereview.chromium.org/34273002/#msg8.

[Nico, 2013-12-06 20:19:32]

On Fri, Dec 6, 2013 at 12:10 PM, <l.gombos@samsung.com> wrote:

> On 2013/12/06 19:40:51, Nico wrote:
>
>> I thought this macro was for -Wunused-param (which we don't use) not
>> -Wunused-variable?
>>
>
> We also (mis)use it for -Werror=unused-variable - see my previous comment -
> https://codereview.chromium.org/34273002/#msg8.
>

Looking through references to it (click "UNUSED_PARAM" in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
look through the list at the bottom), the majority if uses seems to be
for -Wunused-param, and most of these lines can now be deleted.

(The thing in msg8,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
could be fixed by just removing the local variable, and other
instances
might be fixable through ASSERT_UNUSED and the like. If something like
UNUSED_PARAM is in fact still needed, it should get a different name
(chromium's base calls it ignore_result(), but just WTF_UNUSED or similar
would be fine too. Calling it UNUSED_PARAM when it's not supposed to be
used with parameters and most existing uses of it are unnecessary seems
confusing to me)


>
>
>
> https://codereview.chromium.org/34273002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[lgombos, 2013-12-06 20:48:05]

On 2013/12/06 20:19:32, Nico wrote:

> ... the majority if uses seems to be
> for -Wunused-param, and most of these lines can now be deleted.

Ok, thanks for the feedback. I will try to remove as much of the usage of
UNUSED_PARAM as possible and than we can discuss what to do with the rest, but
it seems there is a consensus that in its current form we should try to
eliminate the UNUSED_PARAM macro (and the UnusedParam.h header).

[lgombos, 2013-12-08 16:00:51]

Remove UnusedParam.h.

Nico, what do you think ?

[Nico, 2013-12-08 20:21:32]

Most of the changes in core look good. For the bindings stuff, is it possible to
only generate these variables if they end up being used?

For the leakPtr() calls, I think something like `ignore_result(ptr.leakPtr())`
would be nicer, since that doesn't use up a local name.

But I guess this is already better than what we currently have, so lgtm, and
these things can maybe be addressed in follow-ups.

https://codereview.chromium.org/34273002/diff/530001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/34273002/diff/530001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:618: // FIXME: Implement support
for offset.
Since it's referenced in a comment, maybe keep the parameter name around here

[Inactive, 2013-12-09 15:24:46]

lgtm for bindings/.

BTW, I'm not 100% sure it is worth trying to tweak the bindings generator(s) to
generate instanceTemplate / prototypeTemplate variables only when used. This
will likely increase the complexity of the code generator and IMHO the
readability of the generator is more important that the readability of the
generated bindings code.

As a matter of fact, I have seen recent patches generating more (unneeded) code
to make to bindings generator simpler. The compiler will anyway get rid of the
useless code so this  should not affect binary size.

In any case, this CL does not make the situation worse than it was before (quite
the contrary) so lgtm.

If we really want to instanceTemplate / prototypeTemplate only when used (adding
haraken / nbarth in cc), it should definitely be done in a separate patch
anyway.

https://codereview.chromium.org/34273002/diff/550001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/34273002/diff/550001/Source/bindings/template...
Source/bindings/templates/interface.cpp:481: v8::Local<v8::Signature>
ALLOW_UNUSED defaultSignature = v8::Signature::New(isolate, GetTemplate(isolate,
worldType(isolate)));
Nit: We probably don't need ALLOW_UNUSED here because of the following check a
few lines above:
{% if has_per_context_enabled_methods %}

[haraken, 2013-12-09 15:32:16]

On 2013/12/09 15:24:46, Chris Dumez wrote:
> lgtm for bindings/.
> 
> BTW, I'm not 100% sure it is worth trying to tweak the bindings generator(s)
to
> generate instanceTemplate / prototypeTemplate variables only when used. This
> will likely increase the complexity of the code generator and IMHO the
> readability of the generator is more important that the readability of the
> generated bindings code.

Completely agreed. Improving the maintainability of the IDL compiler would be
more important.

(1) Allow the IDL compiler to generate unused code. Hopefully compilers are
smart enough to remove the unused code.
(2) Observe the binary size (It's already tested in build bots). If the size is
OK, we can just keep the unused code as is.

[lgombos, 2013-12-09 16:39:59]

On 2013/12/08 20:21:32, Nico wrote:

Thanks for all the reviews. I addressed all the feedback with the exception of
the following from Nico, which I plan to do in a followup as he suggested.

> For the leakPtr() calls, I think something like `ignore_result(ptr.leakPtr())`
> would be nicer, since that doesn't use up a local name.

Setting TBR for web/ and modules/ as those changes are the same than what has
been already reviewed in the rest of the CL and are trivial in nature.

[commit-bot: I haz the power, 2013-12-09 16:40:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/l.gombos@samsung.com/34273002/570001

[commit-bot: I haz the power, 2013-12-09 16:40:59]

Failed to apply patch for Source/bindings/scripts/code_generator_v8.pm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/scripts/code_generator_v8.pm
  Hunk #1 succeeded at 4501 (offset -1 lines).
  Hunk #2 succeeded at 4508 (offset -1 lines).
  Hunk #3 FAILED at 4689.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/bindings/scripts/code_generator_v8.pm.rej

Patch:       Source/bindings/scripts/code_generator_v8.pm
Index: Source/bindings/scripts/code_generator_v8.pm
diff --git a/Source/bindings/scripts/code_generator_v8.pm
b/Source/bindings/scripts/code_generator_v8.pm
index
17dcf0c62a3502568c334cf44050b3a8bc0d8949..152f8db4d427f934b700ed0f18091f1c2a0dd5fe
100644
--- a/Source/bindings/scripts/code_generator_v8.pm
+++ b/Source/bindings/scripts/code_generator_v8.pm
@@ -4502,9 +4502,6 @@ END
     $code .= $runtimeEnabledIndent . "        " . ($hasFunctions ?
"${v8ClassName}Methods, WTF_ARRAY_LENGTH(${v8ClassName}Methods),\n" : "0,
0,\n");
     $code .= $runtimeEnabledIndent . "        isolate, currentWorldType);\n";
 
-    AddToImplIncludes("wtf/UnusedParam.h");
-    $code .= "    UNUSED_PARAM(defaultSignature);\n";
-
     if (IsConstructable($interface)) {
         $code .= "   
functionTemplate->SetCallHandler(${v8ClassName}::constructorCallback);\n";
         my $interfaceLength = GetInterfaceLength($interface);
@@ -4512,10 +4509,8 @@ END
     }
 
         $code .=  <<END;
-    v8::Local<v8::ObjectTemplate> instanceTemplate =
functionTemplate->InstanceTemplate();
-    v8::Local<v8::ObjectTemplate> prototypeTemplate =
functionTemplate->PrototypeTemplate();
-    UNUSED_PARAM(instanceTemplate);
-    UNUSED_PARAM(prototypeTemplate);
+    v8::Local<v8::ObjectTemplate> ALLOW_UNUSED instanceTemplate =
functionTemplate->InstanceTemplate();
+    v8::Local<v8::ObjectTemplate> ALLOW_UNUSED prototypeTemplate =
functionTemplate->PrototypeTemplate();
 END
 
     if ($accessCheck) {
@@ -4694,12 +4689,10 @@ END
         $code .= <<END;
 void ${v8ClassName}::installPerContextEnabledMethods(v8::Handle<v8::Object>
prototypeTemplate, v8::Isolate* isolate)
 {
-    UNUSED_PARAM(prototypeTemplate);
 END
         # Define per-context enabled operations.
         $code .=  <<END;
     v8::Local<v8::Signature> defaultSignature = v8::Signature::New(isolate,
GetTemplate(isolate, worldType(isolate)));
-    UNUSED_PARAM(defaultSignature);
 
     ExecutionContext* context =
toExecutionContext(prototypeTemplate->CreationContext());
 END

[commit-bot: I haz the power, 2013-12-09 19:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/l.gombos@samsung.com/34273002/590001

[commit-bot: I haz the power, 2013-12-09 23:36:49]

Change committed as 163467