UNREFERENCED_PARAMETER on apple

May 20, 2014 at 7:30 PM
the define for UNREFERENCED_PARAMETER in apple_compat.h is not preceeded by an #ifndef. This is a problem for projects that already define that macro in a larger context than just where Casablanca is used. Can this be changed?
Coordinator
May 21, 2014 at 11:40 PM
Hi Les1966,

Yes I understand what you are saying and see how this could be an issue. I've opened a bug to track this, we will fix in a future release. In fact I think we can just remove it from the library anyway and directly us 'varname;' instead. If you are interested in contributing back we do accept pull requests. If you sign our CLA you could fix it up.

https://casablanca.codeplex.com/workitem/175

Thanks,
Steve
May 28, 2014 at 6:37 PM
how do I submit the signed CLA?

Also, if we remove the macro, as opposed to correcting it, then in projects that use warning level 4 and warnings as errors will have a problem with unreferenced symbols, won't they, or do I misunderstand what you are saying?
Coordinator
May 28, 2014 at 7:04 PM
Hi Les1966,

Basically you just need to download, sign, and send a copy to either of the following email addresses: stgates at Microsoft dot com or ask Casablanca at Microsoft dot com. Then we can accept your pull requests.

Yes we don't want to start having unreferenced variables producing level 4 warnings. Take a look at what the UNREFERENCED_PARAMETER macro is defined to:
#define UNREFERENCED_PARAMETER(x) (void)x
I don't think it really adds a lot and is conflicting with existing macros in your project. I think it is fine if you delete the macro and in all of its occurrences replace to do the same thing. So for example consider a function like the following:
int MyFunctionFoo(int param1, int param2)
{
    UNREFERENCED_PARAMETER(param2);
    return param1;
}
After deleting the macro Instead change the function to:
int MyFunctionFoo(int param1, int param2)
{
    param2; // unreferenced parameter
    return param1;
}
Obviously this is a made up example, but I think it illustrates the idea. Does that make sense?

Thanks,
Steve
May 28, 2014 at 8:23 PM
That makes sense, personally I find the macro a better method of self documenting code than the inline comment, which is why I have the macro in my cross platform code.

If you think it is a better fix to add the comment, I will do that, but I think it is cleaner ( as much as I dislike macros ) to continue to use the macro and/or to rename the macro to something Casablanca specific.

Lastly, the following code snippet will still produce a warning in xcode on release builds
bool success = SomeFunction();
MY_CUSTOM_ASSERT_MACRO_THAT_IS_A_NOOP_IN_A_RELASE_BUILD(success);
success; // unreferenced parameter
Changing the last line to:
(void) success; // unreferenced parameter
will remove the warning, but it seems less readable to me to add (void) varname; in place of the macro

In short I would rather put an ifndef before the macro, and/or rename the macro unless you strongly disagree with that answer.

Thoughts?
Coordinator
May 28, 2014 at 8:29 PM
Ok yes I agree.

How about this, we keep the macro but instead give it a name that won't conflict with other libraries. How about we call it CASABLANCA_UNREFERENCED_PARAMETER? The name is a bit long, but it shouldn't give us any troubles in the future.

You will need to make the changes in apple_compat.h, linux_compat.h, and windows_compat.h.

Thanks,
Steve
Coordinator
May 28, 2014 at 11:50 PM
Your pull request has been merged. Thanks for the change!

Keep in mind now that you've signed the CLA you can submit future pull requests as well.

Steve
Jun 19, 2014 at 4:18 PM
Seems like this change was lost in the shuffle, before I resubmit it on the latest code base, would you rather I bundle it up in an ifdef as opposed to reintroducing CASABLANCA_UNREFERENCED_PARAMETER?
Coordinator
Jun 19, 2014 at 5:45 PM

Hi Les1966,

When I look at the history in git I can see your commit. It actually only modified the linux and windows compat but not apple_compat.h. I don’t think you need to introduce it under and ifdef, how about you submit a pull request fixing it up in the apple header?

Ideally in the future we probably should combine all of the compat header files into one file…

Thanks,

Steve

Jun 19, 2014 at 9:14 PM
Cleaned it up, request submitted, thanks Steve.