Sign in to follow this  
Alundra

Unused param

Recommended Posts

Hi,

I always used this macro to remove warning when needed :

#define UNUSED(x) static_cast<void>(x)

But I recently see, a template version could be used like that :

template<class T> INLINE void UNUSED(const T&){}

What is the best to use and why ? I read Intel compiler didn't like static_cast to remove warning, but apparently now it does from other post.

Thanks

Edited by Alundra

Share this post


Link to post
Share on other sites

Unless I'm in the situation where the parameter is used in only debug or only release build, I tend to just comment out the parameter name.

 

I've also seen people just mention the variable name in the function body on a line by itself, without any cast.

Share this post


Link to post
Share on other sites
void FunkySmell( int _iUnused ) {
}
// Warning, unused parameter.

void FunkySmell( int _iUnused ) {
    static_cast<void>(_iUnused);
}
// Warning, useless statement (no side-effect).

void FunkySmell( int /*_iUnused*/ ) {
}
// Correct.

L. Spiro Edited by L. Spiro

Share this post


Link to post
Share on other sites

I see.
But I am fairly sure (entirely open to correction) that your cast produces the same result as my static cast, which generates a warning in Xcode.
No matter what, Coverity is going to flag these things.


I’m not sure of any way to avoid these kinds of warnings entirely (and we are not having a discussion on why we are putting “useless” parameters into functions because there are so many legitimate reasons why it happens), so your best bet is just to figure out what reduces the number of warnings you yourself get while compiling.

Hodgman and I have our favorite ways. They don’t always eliminate the problems, especially when you are supporting lots of platforms or even just overloading operator new.  Right now your best course of action is simply to do what it takes to remove the warnings that you are encountering.  I stand by my proposal, but I literally don’t think any course of action will pacify every compiler out there.  For now it makes more sense to just do what pacifies most of them, and if you encounter some remaining issues then ask again, specifying which compiler you are using and what warning you aim to avoid.

 

 

L. Spiro

Share this post


Link to post
Share on other sites

If no real solution, an if-else checking the compilator can always gives the trick, actually static_cast remove all of these warnings on msvc,gcc and clang.

I didn't test myself intel compilator but apparently now the template version is not needed, the static_cast seems to remove the warning too.

Edited by Alundra

Share this post


Link to post
Share on other sites

With VC++ I use the macro UNREFERENCED_PARAMETER(x).

 

I've just had a quick peek and it's defined as the following.

#define UNREFERENCED_PARAMETER(P)          (P)

Edited by collie

Share this post


Link to post
Share on other sites


and we are not having a discussion on why we are putting “useless” parameters into functions because there are so many legitimate reasons why it happens

I'm not sure I buy the 'legitimate' part of that. As I see it, there are usually two cases where we end up with unused parameters:

  • When the implementation has changed such that the public API no longer accurately reflects the provided functionality.
  • When you abuse polymorphism such that a child must override a parent method to do something unrelated.

Neither of those are particularly legitimate - the former can be addressed via deprecation, and the latter by refactoring.

Share this post


Link to post
Share on other sites

Neither of those are particularly legitimate - the former can be addressed via deprecation, and the latter by refactoring.

We work in games though, where we're shipping products that are over-ambitious and under-budgeted. Software engineering becomes less about perfect ideals, and more about an optimal strategy to deal with sub-optimal resources wink.png ..... which means it's perfectly legitimate to choose to form a strategy that manages -- not rewrites -- bad code sad.png
 

I see UNUSED macros more often used for local variables than arguments though. Often there's some debug code that's only required for an assertion -- e.g.

bool ok = Frobnicate();
ASSERT( ok );
UNUSED( ok );

Say that the only reason Frobnicate can fail is due to a logic error (and we can't ship the game if there's a logic error) so the retail version has no need to examine the return code.
In the development version, ASSERT validates our assumption that the function can't fail, and in the retail version UNUSED suppresses the warning resulting from ignoring the return code (because we're responsible engineers and use warnings-as-errors and warning-level-4 biggrin.png).

 

Often unused params to functions serve the same purpose -- e.g. with your example of abused polymorphism, where one implementation doesn't quite match the interface: Often in such cases, this implementation will perform assertions on the otherwise unused argument, but still need to suppress the unused-argument warning in retail builds.

e.g. maybe most platforms require this parameter, but one platform doesn't require it... but that platform can still provide a nice development environment by checking that the value will also work fine for the rest of your platforms.

Edited by Hodgman

Share this post


Link to post
Share on other sites


because we're responsible engineers and use warnings-as-errors and warning-level-4

That's already a fallacy. You've explicitly decided to accept a bunch of restrictive warnings as errors, and rather than fixing those errors, you are willing to paper them over and ignore them.

 

There's no earthly reason to enable the unused parameter warning as an error, if you aren't going to treat it as if it were an actual error condition. At that point you are just pandering to the foibles of the tool at hand.

Share this post


Link to post
Share on other sites

because we're responsible engineers and use warnings-as-errors and warning-level-4

That's already a fallacy. You've explicitly decided to accept a bunch of restrictive warnings as errors, and rather than fixing those errors, you are willing to paper them over and ignore them.
 
There's no earthly reason to enable the unused parameter warning as an error, if you aren't going to treat it as if it were an actual error condition. At that point you are just pandering to the foibles of the tool at hand.


As a responsible engineer, it is my responsibility to determine if the compiler is being overly strict, or whether it caught a potential bug. It is also my responsibility to estimate the cost of fixing the warning vs the cost of "papering over" the warning. At that point I can make an informed choice, or inform a producer who can make the call based on the state of the project.

Sometimes you need to ship a product and the risk of re-architecting your game to make warnings go away isn't worth the benefit of removing a warning that is showing you something harmless.

You don't ignore the tool - but you don't let the tool control you either smile.png

Share this post


Link to post
Share on other sites

When was the last time any of you had an unused parameter warning that actually represented a bug? I can't recall such an instance, myself.

 

In which case, adding an [tt]-Wno-unused-variable[/tt] (or equivalent) to your build system seems a lot less disruptive than littering the code with [tt]UNUSED()[/tt] macros (or any of the other less discoverable alternatives).

Share this post


Link to post
Share on other sites

I usually enable unused parameter/variable warnings, and use an UNUSED macro or template to suppress them. One frequent use is during development, where I leave the warnings for partially completed functionality or where I'm not sure yet if I want to use a particular variable. After completing the function and realizing the parameter actually isn't needed I add UNUSED to disable warnings if I can't just remove the parameter entirely.

Of course I could just add #pragma warnings when creating the function instead.. but it's nice to know I didn't forget any variables, and only set them as unused when having properly checked the function. Also might leave parameters for future use where it's far enough into the future that I don't want a constant warning, but know I will sometime come back to that function.

 

I actually would like an unused-attribute that gives an error if the parameter marked unused is used later, to force me to remove the UNUSED if I later add functionality that uses the parameter. Those UNUSED-markers is a pretty nice way of documenting what a function does and doesn't do sometimes.

(For this case commenting out the name with /* name */ would work well).

 

Also useful for mock-interfaces or debug functions.

Like virtual result networkRequest(url, port, protocol), and the testing-interface just returns a static result and uses UNUSED on parameters not used, or for base-classes that do nothing in some virtual method that I still don't want pure for efficiency in writing simple classes that uses the base.

 

I'm sure there will be a theoretically better way pretty much every time, but I find both that it's often efficient to allow some parameters or return values to remain unused, and that the warning itself is useful.

 

In the case of an unused return variable it's a helpful documentation in the calling code to whoever reads the code the next time, and to give errors or warnings if the return type was to be changed at a later time... though in that case I have a tendency to comment out the return variable instead of using UNUSED.

Edited by Erik Rufelt

Share this post


Link to post
Share on other sites

When was the last time any of you had an unused parameter warning that actually represented a bug? I can't recall such an instance, myself.
 
In which case, adding an [tt]-Wno-unused-variable[/tt] (or equivalent) to your build system seems a lot less disruptive than littering the code with [tt]UNUSED()[/tt] macros (or any of the other less discoverable alternatives).


Comes up all the time when I'm making changes to an already existing function and modify it in such a way that I don't need some of the parameters that used to be passed to it. It's a good reminder to remove the parameter and clean up the callers.

Share this post


Link to post
Share on other sites

because we're responsible engineers and use warnings-as-errors and warning-level-4

That's already a fallacy. You've explicitly decided to accept a bunch of restrictive warnings as errors, and rather than fixing those errors, you are willing to paper them over and ignore them.

There's no earthly reason to enable the unused parameter warning as an error, if you aren't going to treat it as if it were an actual error condition. At that point you are just pandering to the foibles of the tool at hand.

That would be fair, if you always ignored the error except to treat it as an instruction to insert the UNUSED macro.
This is not the case.
As with all warnings, usually you take their advice, but sometimes you overrule them and document that you're intentionally doing something smelly.

Compiler errors tell you that toy do have a syntax or logic error.
Warnings tell you that you probably have a logic error, but maybe dont.

If a warning is a false alarm, there's usualy a very small tweak that can be made that not only supresses the warning, but also documents your assumtion for other programmers.
e.g.
u32 a = 0x12345678;
u16 b = (u16)a; // warning - possible data loss
u16 c = (u16)(a & 0xFFFF); // no warning, plus exact same code generation (the AND is optimized out)
//The no-warning version also explicitly documents the programmer's intention, making the slicing obvious, making the logic more readable
This goes for static code analysis too:
void* buffer = malloc( sizeof(Header) + sizeof(Content));
memset(buffer, 0, sizeof(Header)); // warning! - partial buffer fill
....
So we add a comment to disable that static warning, and explain the smell to fellow coders who might also pick up on it:
....
memset(buffer, 0, sizeof(Header)); //-W1234 -- deliberately only clearing the header, as it's assumed the caller will complete overwite the content section.
Having the warnings enabled in the first place is important, so that these potential bugs/smells are pointed out.

Having warnings alone is often useless though, as once more than a handful are comitted to the project, people start to ignore them, and ignore any new ones they write, defeating the purpose.
So warnings-as-errors is used to ensure they're not ignored - you must clean up any automatically detected code smells before you can even commit your code to the main branch.

When was the last time any of you had an unused parameter warning that actually represented a bug?

Similar to what you said earlier, they're not usually bugs, but code smells -- "you've refactored this code so a parameter is no longer needed, are you sure you don't want to clean up the interface too?".

As above, if after giving thought to that automated question you decide that, no, you don't want to clean up the interface for whatever reason, then you can leave behind an UNUSED macro and a comment documenting your decision, so that the compiler and your colleagues know it was an intentional choice. Edited by Hodgman

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this