Upstreaming changes from libchrome in AOSP: -Wunused-parameter

[Luis Hector]

Hi,

I'm uprevving libchrome[1] in AOSP and since Android is built with different warning enabled, there is a large set of local patches that make updating to ToT Chrome difficult and time-consuming. Since I'd like to make the next uprevver's life easier, I have an open review[2] that upstreams some of the modifications that make sense, and complies with Google's C++ style[3], but I was suggested to post here to discuss one type of changes in particular:

Android builds with -Wunused-*, whereas Chromium does not. Google's style guide in https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions mentions that "Unused parameters that are obvious from context may be omitted [... and ...] Unused parameters that might not be obvious should comment out the variable name in the function definition". I did find a few places here and there in components/, content/, chrome/, where this happens[4], but the vast majority of places add the parameter name even if it is unused.

While I can probably try to push to add -Wno-unused-* in AOSP side to avoid the need for these changes, there are still some headers that get included in other projects, and adding those flags in more places is going to be a tough sell, so the easiest thing is still to try to uprev these changes to Chrome.

Looking for guidance and suggestions,

Luis

1: https://android.googlesource.com/platform/external/libchrome/+/master

2: https://codereview.chromium.org/1997153002/

3: https://google.github.io/styleguide/cppguide.html

4: ag '[&*]\) .*{' -G cc$ components content chrome shows ~hundreds of matches, although some are false positives.

[Alexei Svitkine]

For more context, here's the CL that's currently under review: https://codereview.chromium.org/1997153002/

In that CL, a lot of function definitions get changed in the following manner:

bool SampleCountIterator::GetBucketIndex(size_t* index) const {

to:

bool SampleCountIterator::GetBucketIndex(size_t* /*index*/) const {

I raised the concern that this does not match currently convention in Chromium browser code and even though it is allowed by Google C++ Style Guide (just like other things that the guide allows but we don't do - such as writing "Foo *bar" instead of "Foo* bar"), we shouldn't just do this in Chromium before there's consensus on chromium-dev@ that we should.

So what do others think? Should we allow this and have an inconsistency where some files use this convention or others don't?

Personally, I prefer writing out the parameters so its obvious what the mean (i.e. not just omitting them) and the extra /**/ comment is just extra noise on top compared to just having the param name. But I'm happy to bend my opinion if there's support for allowing it.

-Alexei

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Dmitry Skiba]

That CL uses 3 strategies:

1. Omit parameter name.

2. Comment out parameter name (in case parameter type is too generic, like int).

3. Use (void)param; to silence the warning.

It seems that last two can be merged together - if you want to name parameter which is not used, explicitly mark it as such in the method. I would also prefer to have dedicated macro for that (UNUSED?) instead of repeating "(void)param; // Avoid the warning" thing. So with that there will be only two strategies:

1. Omit parameter name.

2. Mark parameter as UNUSED() inside method body.

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Antoine Labour]

On Wed, May 25, 2016 at 10:10 AM, Alexei Svitkine <asvi...@chromium.org> wrote:

For more context, here's the CL that's currently under review: https://codereview.chromium.org/1997153002/

In that CL, a lot of function definitions get changed in the following manner:

bool SampleCountIterator::GetBucketIndex(size_t* index) const {

to:

bool SampleCountIterator::GetBucketIndex(size_t* /*index*/) const {

I raised the concern that this does not match currently convention in Chromium browser code and even though it is allowed by Google C++ Style Guide (just like other things that the guide allows but we don't do - such as writing "Foo *bar" instead of "Foo* bar"), we shouldn't just do this in Chromium before there's consensus on chromium-dev@ that we should.

So what do others think? Should we allow this and have an inconsistency where some files use this convention or others don't?

Personally, I prefer writing out the parameters so its obvious what the mean (i.e. not just omitting them) and the extra /**/ comment is just extra noise on top compared to just having the param name. But I'm happy to bend my opinion if there's support for allowing it.

-Alexei

The names of unused parameters are mostly useful to the readers of the interface. Could a compromise be to leave the name in the declaration, but omit them in the function definition? Assuming that doesn't trigger the warning of course.

Antoine

 

On Tue, May 24, 2016 at 7:35 PM, Luis Hector <lhch...@chromium.org> wrote:

Hi,

I'm uprevving libchrome[1] in AOSP and since Android is built with different warning enabled, there is a large set of local patches that make updating to ToT Chrome difficult and time-consuming. Since I'd like to make the next uprevver's life easier, I have an open review[2] that upstreams some of the modifications that make sense, and complies with Google's C++ style[3], but I was suggested to post here to discuss one type of changes in particular:

Android builds with -Wunused-*, whereas Chromium does not. Google's style guide in https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions mentions that "Unused parameters that are obvious from context may be omitted [... and ...] Unused parameters that might not be obvious should comment out the variable name in the function definition". I did find a few places here and there in components/, content/, chrome/, where this happens[4], but the vast majority of places add the parameter name even if it is unused.

While I can probably try to push to add -Wno-unused-* in AOSP side to avoid the need for these changes, there are still some headers that get included in other projects, and adding those flags in more places is going to be a tough sell, so the easiest thing is still to try to uprev these changes to Chrome.

Looking for guidance and suggestions,

Luis

1: https://android.googlesource.com/platform/external/libchrome/+/master

2: https://codereview.chromium.org/1997153002/

3: https://google.github.io/styleguide/cppguide.html

4: ag '[&*]\) .*{' -G cc$ components content chrome shows ~hundreds of matches, although some are false positives.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Luis Hector Chavez]

On Wednesday, May 25, 2016 at 12:12:45 PM UTC-7, Antoine Labour wrote:

On Wed, May 25, 2016 at 10:10 AM, Alexei Svitkine <asvi...@chromium.org> wrote:

For more context, here's the CL that's currently under review: https://codereview.chromium.org/1997153002/

In that CL, a lot of function definitions get changed in the following manner:

bool SampleCountIterator::GetBucketIndex(size_t* index) const {

to:

bool SampleCountIterator::GetBucketIndex(size_t* /*index*/) const {

I raised the concern that this does not match currently convention in Chromium browser code and even though it is allowed by Google C++ Style Guide (just like other things that the guide allows but we don't do - such as writing "Foo *bar" instead of "Foo* bar"), we shouldn't just do this in Chromium before there's consensus on chromium-dev@ that we should.

So what do others think? Should we allow this and have an inconsistency where some files use this convention or others don't?

Personally, I prefer writing out the parameters so its obvious what the mean (i.e. not just omitting them) and the extra /**/ comment is just extra noise on top compared to just having the param name. But I'm happy to bend my opinion if there's support for allowing it.

-Alexei

The names of unused parameters are mostly useful to the readers of the interface. Could a compromise be to leave the name in the declaration, but omit them in the function definition? Assuming that doesn't trigger the warning of course.

Antoine

Leaving unused parameters in the declaration does not trigger the warning. I'm fine with either this strategy or the UNUSED macro within the function body. What do people prefer? Personally, I'm a bit more inclined towards the macro.

[Colin Blundell]

I have always been glad that Chromium does not treat unused parameters specially.

My opinion is that:

- Treating them specially creates visual noise (The question of "Is this parameter used or not?" is one I basically never find myself asking).

- It adds an annoyance when using the parameter that you will likely get a compile error and then have to undo the "mark the parameter as unused" bit.

Do others find value in this convention, or is the concrete question here basically entirely about making Chromium easier to consume by AOSP?

[Peter Kasting]

I am opposed to making changes to avoid unused param warnings, in this case or in general.

The reason for this is that if nearly all Chromium code does not treat unused params specially, but a little bit does,

* The inconsistency makes doing correct reviews difficult

* Files that Android no longer pulls in no longer need this exception, but will doubtless continue to have it

* Files and changes that get newly pulled into Android will continue to break it unless we get some sort of bot in the normal try rotation that builds in such a way as to catch this

These problems go away if we change the entire codebase to comment out unused params, but then we have other problems:

* The benefit of this warning is questionable.  The change I'm making to use =delete for DISALLOW_COPY_AND_ASSIGN is much more effective at exposing unused variables, for example.

* Silencing the warning is often complicated, as you've noticed, by things like #ifdefs that are the only uses of a variable within a function; there are no really good ways to handle this, as one needs to either add #ifdefs around the function parameters themselves to conditionally comment them, or add #else cases, ugly macros, etc. to the function body.  These hinder readability and pose a risk of not being cleaned up later.

* As already noted, the param name must be restored later if someone wants to use it, which is admittedly a very minor annoyance.

The Google style guide also does not mandate that unused parameters _must_ be commented out (or the names removed entirely); it uses "may" in these cases.  So Chromium's current behavior does not represent a deviation from the Google style guide, merely the outcome of a different set of compiler flag choices than Google style.  (If we were violating the Google style guide here, there would be a stronger argument for changing Chromium, even if that change were difficult.)

It is common in Chromium to import third-party code and need to set different warning flags for that code, as third-party projects often have different style and different rules than Chromium code.  I think it is reasonable to expect that other projects which include Chromium code pay us the same courtesy in cases like this where I think the costs of trying to "fix" this warning are non-negligible.

Also, you mention "headers that get included in other projects", but as noted elsewhere on this thread, the issue is not with function declarations including a name, but with function definitions that don't use the param.  In most cases, definitions will be in .cc files, so including headers elsewhere does not matter.  The exception is when the functions in question are actually defined in the header.  In these cases, we may be able to fix some instances by changing the function signature to take fewer params, or by moving the definition to a .cc file (if this does not result in a performance penalty).  I would hope the number of remaining issues not solvable in these ways is low enough not to cause other projects problems, but we could perhaps consider _just_ fixing these few cases, depending on the extent.

PK

[Primiano Tucci]

TL;DR It seems to me that chromium has explicitly opted out from -Wunused-parameter. So either we reiterate this decision and agree to remove the -Wno-unused-parameter to avoid regressing, or you should add -Wno-unused-parameter to your Android.mk. To me it doesn't make a lot of sense saying "in chromium we explicitly disable warnings about unused parameters but every now and then we fix them manually"

> Android builds with -Wunused-*, whereas Chromium does not.

This is not true. Chrome does build with -Wunused-variable. More generally we build with -Wall and opt out from specific warnings.

The major exception is non-chromium code (third party libraries), in which case you are out of luck anyways (unless you don't have any third_party in your Android fork): most of these subprojects are pure mirrors of open source projects. So the only option there is convincing the owners of all those projects and change their code.

The thing we opt-out here is -Wno-unused-parameter, which seems deliberate.

In other words, we do have (fatal) warnings for unused variables. We just don't have them for unused arguments (which IMHO is a very reasonable tradeoff).

Honestly, I don't understand one point: you are importing chromium code into AOSP. Why don't you just match the compiler flags that we use upstream? This is what WebView was doing when WebView was in the Android tree. It makes maintenance of your project way easier (for some definition of "easy" w.r.t. maintaining a chromium fork in Android)

In other words: what is the downside of adding  -Wno-unused-parameter into your Android.mk?

For the sake of this discussion, I don't have an opinion on Foo(int /*arg*/) vs Foo(int arg). On the other side the option I really dislike:

Foo(int arg) {

  ignore_result(arg);  

  ...

}

This adds really unnecessary clutter, increases the #lines of the file, and is unnecessary developer pain.

P.S. we have that macro already in base/macros.h, it's not a macro and is called ignore_result ;-)

--

[Peter Kasting]

On Thu, May 26, 2016 at 3:07 AM, Primiano Tucci <prim...@chromium.org> wrote:

P.S. we have that macro already in base/macros.h, it's not a macro and is called ignore_result ;-)

While the compiler ought to be able to optimize this away, I would worry that in some scenarios ignore_result()['s implementation] might defeat some kind of optimization more than (void)var; would.

PK 

[Ken Rockot]

Was there ever a decision here on what to do for https://codereview.chromium.org/1997153002/? I think I generally agree with Primiano's response that libchrome should not be built with this warning enabled, rather than having us upstream changes to the code which avoid triggering the warning.

Enabling the warning on upstream builds would be a lot of churn, and (for good reasons) it doesn't seem like there's any interest in doing that. Not having it enabled upstream means we'll continue to introduce unused parameters and you'll continue to run into the issue when you uprev. As Primiano asked, what's the downside to changing your build flags?

--

[Nico Weber]

I'm guessing the problem is that AOSP's upstream code builds its code with -Wunused-parameter but includes chromium's headers – so AOSP translation units will warn about this in Chromium's headers. Luis, can you change AOSP to set the include path to chromium's headers using -isystem instead of -I? Then AOSP won't see warnings from Chromium's headers (the compiler will treat Chromium's headers as system headers then, and it doesn't warn on things in system headers).