Any objections to turning on -Wunused-parameter? (and any other -Wunused-* that are off?)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Any objections to turning on -Wunused-parameter? (and any other -Wunused-* that are off?)

Chandler Carruth-2
LLVM and Clang both have lots of objects that are passed through many different API boundaries. Things like AliasAnalysis in LLVM or Sema in Clang get threaded all over the place.

Over times, refactoring can often cause the parameters (or local variables, or member variables, etc) to become dead. If we notice this, we can often un-thread the interface through our APIs, sometimes even reducing coupling, etc.

But currently -Wunused-parameter is hard disabled. I've not checked to see if there are any other disabling of -Wunused-* variants, but I'd like to move the project toward firmly enabling them and being clean with them.

The technique I'd like to use is leaving out names of unused parameters, using a /*CommentedName*/ if there is a useful name, otherwise just omitting the name.

All of this will require a reasonable amount of cleanup across the projects which I'm happy to do prior to flipping defaults around. Thoughts? Any concerns or objections?

(I'll ask the same question and ofter to either enact the cleanups or toggle the warning(s) back off for each of the less intertwined subprojects like LLD, LLDB, Polly, etc.)

-Chandler

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Any objections to turning on -Wunused-parameter? (and any other -Wunused-* that are off?)

Renato Golin Linaro
On 18 July 2015 at 10:48, Chandler Carruth <[hidden email]> wrote:
> But currently -Wunused-parameter is hard disabled. I've not checked to see
> if there are any other disabling of -Wunused-* variants, but I'd like to
> move the project toward firmly enabling them and being clean with them.

+1


> The technique I'd like to use is leaving out names of unused parameters,
> using a /*CommentedName*/ if there is a useful name, otherwise just omitting
> the name.

I'd just get rid of everything. Often, outdated comments can be more
harmful than no comments.

cheers,
--renato
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Any objections to turning on -Wunused-parameter? (and any other -Wunused-* that are off?)

Chris Lattner
In reply to this post by Chandler Carruth-2

> On Jul 18, 2015, at 2:48 AM, Chandler Carruth <[hidden email]> wrote:
>
> LLVM and Clang both have lots of objects that are passed through many different API boundaries. Things like AliasAnalysis in LLVM or Sema in Clang get threaded all over the place.
>
> Over times, refactoring can often cause the parameters (or local variables, or member variables, etc) to become dead. If we notice this, we can often un-thread the interface through our APIs, sometimes even reducing coupling, etc.
>
> But currently -Wunused-parameter is hard disabled. I've not checked to see if there are any other disabling of -Wunused-* variants, but I'd like to move the project toward firmly enabling them and being clean with them.
>
> The technique I'd like to use is leaving out names of unused parameters, using a /*CommentedName*/ if there is a useful name, otherwise just omitting the name.
>
> All of this will require a reasonable amount of cleanup across the projects which I'm happy to do prior to flipping defaults around. Thoughts? Any concerns or objections?

+1.  We should also consider other general goodness warnings as well.

-Chris


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] Any objections to turning on -Wunused-parameter? (and any other -Wunused-* that are off?)

Philip Reames
In reply to this post by Chandler Carruth-2
I'd be very happy to see this turned on as a warning; I'd be very reluctant to see it turned on as a error.  During a lot of code cleanups, the intermediate state of "deleted, but dead args still passed in" is a very useful intermediate step.  It greatly simplifies review to be able to separate the meat of the semantic change from the "just deleting a bunch of dead args" patch.  I'd be very very reluctant to create a situation where the patch with the semantic change but without the argument cleanup won't pass all the build bots. 

Philip

(1 inline comment below as well)

On 07/18/2015 02:48 AM, Chandler Carruth wrote:
LLVM and Clang both have lots of objects that are passed through many different API boundaries. Things like AliasAnalysis in LLVM or Sema in Clang get threaded all over the place.

Over times, refactoring can often cause the parameters (or local variables, or member variables, etc) to become dead. If we notice this, we can often un-thread the interface through our APIs, sometimes even reducing coupling, etc.

But currently -Wunused-parameter is hard disabled. I've not checked to see if there are any other disabling of -Wunused-* variants, but I'd like to move the project toward firmly enabling them and being clean with them.

The technique I'd like to use is leaving out names of unused parameters, using a /*CommentedName*/ if there is a useful name, otherwise just omitting the name.
Sorry, could you rephrase?  Not sure what you're getting at here.

All of this will require a reasonable amount of cleanup across the projects which I'm happy to do prior to flipping defaults around. Thoughts? Any concerns or objections?

(I'll ask the same question and ofter to either enact the cleanups or toggle the warning(s) back off for each of the less intertwined subprojects like LLD, LLDB, Polly, etc.)

-Chandler


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] Any objections to turning on -Wunused-parameter? (and any other -Wunused-* that are off?)

David Blaikie


On Mon, Jul 20, 2015 at 4:18 PM, Philip Reames <[hidden email]> wrote:
I'd be very happy to see this turned on as a warning; I'd be very reluctant to see it turned on as a error.  During a lot of code cleanups, the intermediate state of "deleted, but dead args still passed in" is a very useful intermediate step.  It greatly simplifies review to be able to separate the meat of the semantic change from the "just deleting a bunch of dead args" patch.  I'd be very very reluctant to create a situation where the patch with the semantic change but without the argument cleanup won't pass all the build bots. 

Yep - I recall Chandler expressing similar consternation when he & I have discussed -Wunreachable-code so I was a bit surprised to see the suggestion.

But that said, in this case it doesn't seem too costly to comment out the name of the parameter in the first patch - it is an extra change, but doesn't require touching all the callers, etc. Then do the removal in a follow up patch still.
 

Philip

(1 inline comment below as well)

On 07/18/2015 02:48 AM, Chandler Carruth wrote:
LLVM and Clang both have lots of objects that are passed through many different API boundaries. Things like AliasAnalysis in LLVM or Sema in Clang get threaded all over the place.

Over times, refactoring can often cause the parameters (or local variables, or member variables, etc) to become dead. If we notice this, we can often un-thread the interface through our APIs, sometimes even reducing coupling, etc.

But currently -Wunused-parameter is hard disabled. I've not checked to see if there are any other disabling of -Wunused-* variants, but I'd like to move the project toward firmly enabling them and being clean with them.

The technique I'd like to use is leaving out names of unused parameters, using a /*CommentedName*/ if there is a useful name, otherwise just omitting the name.
Sorry, could you rephrase?  Not sure what you're getting at here.

All of this will require a reasonable amount of cleanup across the projects which I'm happy to do prior to flipping defaults around. Thoughts? Any concerns or objections?

(I'll ask the same question and ofter to either enact the cleanups or toggle the warning(s) back off for each of the less intertwined subprojects like LLD, LLDB, Polly, etc.)

-Chandler


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] Any objections to turning on -Wunused-parameter? (and any other -Wunused-* that are off?)

Chandler Carruth
LLVM has a separate flag for making warnings errors, and I'm not proposing to change this settings default.

I'm satisfied with the fact that when submitted, a patch is expected to be free of warnings.

On Mon, Jul 20, 2015 at 4:52 PM David Blaikie <[hidden email]> wrote:
On Mon, Jul 20, 2015 at 4:18 PM, Philip Reames <[hidden email]> wrote:
I'd be very happy to see this turned on as a warning; I'd be very reluctant to see it turned on as a error.  During a lot of code cleanups, the intermediate state of "deleted, but dead args still passed in" is a very useful intermediate step.  It greatly simplifies review to be able to separate the meat of the semantic change from the "just deleting a bunch of dead args" patch.  I'd be very very reluctant to create a situation where the patch with the semantic change but without the argument cleanup won't pass all the build bots. 

Yep - I recall Chandler expressing similar consternation when he & I have discussed -Wunreachable-code so I was a bit surprised to see the suggestion.

But that said, in this case it doesn't seem too costly to comment out the name of the parameter in the first patch - it is an extra change, but doesn't require touching all the callers, etc. Then do the removal in a follow up patch still.
 

Philip

(1 inline comment below as well)

On 07/18/2015 02:48 AM, Chandler Carruth wrote:
LLVM and Clang both have lots of objects that are passed through many different API boundaries. Things like AliasAnalysis in LLVM or Sema in Clang get threaded all over the place.

Over times, refactoring can often cause the parameters (or local variables, or member variables, etc) to become dead. If we notice this, we can often un-thread the interface through our APIs, sometimes even reducing coupling, etc.

But currently -Wunused-parameter is hard disabled. I've not checked to see if there are any other disabling of -Wunused-* variants, but I'd like to move the project toward firmly enabling them and being clean with them.

The technique I'd like to use is leaving out names of unused parameters, using a /*CommentedName*/ if there is a useful name, otherwise just omitting the name.
Sorry, could you rephrase?  Not sure what you're getting at here.

All of this will require a reasonable amount of cleanup across the projects which I'm happy to do prior to flipping defaults around. Thoughts? Any concerns or objections?

(I'll ask the same question and ofter to either enact the cleanups or toggle the warning(s) back off for each of the less intertwined subprojects like LLD, LLDB, Polly, etc.)

-Chandler


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev