rfc: winnt.h's UNREFERENCED_PARAMETER() vs clang's -Wunused-value

Hi,

clang has a useful warning -Wunused-value that warns on things such as 4; (this probably meant to say return 4;).

winnt.h has a macro UNREFERENCED_PARAMETER(x) that’s used to mark unused function parameters. It’s somewhat commonly used on Windows, for example the default project generated by MSVC uses it a few times. The macro is defined as #define UNREFERENCED_PARAMETER(x) (x), which means if you use it like it’s intended:

void f(int x) {
UNREFERENCED_PARAMETER(x);
}

then clang will emit a -Wunused-value warning for it. How do we want to fix this? Ideas:

  1. Don’t. Tell people to not use UNREFERENCED_PARAMETER().
  2. Have a winnt.h wrapper in lib/Headers that undefines this macro, include_next’s regular winnt.h, undefines it again, and then defines it to something that doesn’t trigger -Wunused-value (say, (void)p, or do { (void) P; } while (0) or similar).
  3. Change clang’s Wunused-value to check if the unused value is expanded from a macro that was defined in a system header that’s spelled UNREFERENCED_PARAMETER.

2 sounds like it has the highest chance of unintended side effects. 3 is a bit hacky, but seems nicest from a user’s perspective and isn’t worse than other things we do really.

Other ideas? Opinions?

Thanks,
Nico

(2) is what people have been told to use on other systems, at least the
cast to void part.

Joerg

I’d really rather not do 2. Fighting with MSVC CRT headers over va_start has been pretty awkward. Users have reported issues with our header override fails. I’d rather not start fighting with winnt.h.

I’m OK with 3, but I’m desensitized to having to do this kind of pattern matching for __uuidof, etc. Maybe we should wait for another +1. :slight_smile:

I'd really rather not do 2. Fighting with MSVC CRT headers over va_start
has been pretty awkward. Users have reported issues with our header
override fails. I'd rather not start fighting with winnt.h.

I'm OK with 3, but I'm desensitized to having to do this kind of pattern
matching for __uuidof, etc. Maybe we should wait for another +1. :slight_smile:

A +1 from me for option #3 as well. I'd feel more comfortable doing #2 if
it was for correctness (along the lines of our va_start/va_arg hack).

On what basis does MSVC decide that UNREFERENCED_PARAMETER should silence
an unused parameter? IIRC, it expands to wrapping it's argument in
parenthesis. Could we teach clang to do the same? Sounds like it could be
a more general fix.

I'd really rather not do 2. Fighting with MSVC CRT headers over va_start
has been pretty awkward. Users have reported issues with our header
override fails. I'd rather not start fighting with winnt.h.

I'm OK with 3, but I'm desensitized to having to do this kind of pattern
matching for __uuidof, etc. Maybe we should wait for another +1. :slight_smile:

A +1 from me for option #3 as well. I'd feel more comfortable doing #2 if
it was for correctness (along the lines of our va_start/va_arg hack).

On what basis does MSVC decide that UNREFERENCED_PARAMETER should silence
an unused parameter? IIRC, it expands to wrapping it's argument in
parenthesis. Could we teach clang to do the same? Sounds like it could be
a more general fix.

That sounds like a really good idea to me – teach sema that "(x);" has the
same meaning as "(void)x;".

I'd really rather not do 2. Fighting with MSVC CRT headers over va_start
has been pretty awkward. Users have reported issues with our header override
fails. I'd rather not start fighting with winnt.h.

I'm OK with 3, but I'm desensitized to having to do this kind of pattern
matching for __uuidof, etc. Maybe we should wait for another +1. :slight_smile:

A +1 from me for option #3 as well. I'd feel more comfortable doing #2 if
it was for correctness (along the lines of our va_start/va_arg hack).

On what basis does MSVC decide that UNREFERENCED_PARAMETER should silence
an unused parameter? IIRC, it expands to wrapping it's argument in
parenthesis. Could we teach clang to do the same? Sounds like it could be
a more general fix.

That sounds like a really good idea to me – teach sema that "(x);" has the
same meaning as "(void)x;".

I think that's a reasonable approach, but #3 is a good fallback position IMO.

~Aaron

Hold on a moment.

`(x);` certainly seems like it should silence an "unreferenced parameter"
warning (which is what this macro is for in MSVC), but it seems much less
reasonable for it to suppress an "unused value" warning (which is the
warning we're getting with Clang). This is not an idiom we should encourage
for expressing "I am deliberately discarding this value", so I would prefer
limiting this to the case where the parens come from a macro named
UNREFERENCED_PARAMETER.

I'd really rather not do 2. Fighting with MSVC CRT headers over
va_start has been pretty awkward. Users have reported issues with our
header override fails. I'd rather not start fighting with winnt.h.

I'm OK with 3, but I'm desensitized to having to do this kind of
pattern matching for __uuidof, etc. Maybe we should wait for another +1. :slight_smile:

A +1 from me for option #3 as well. I'd feel more comfortable doing #2
if it was for correctness (along the lines of our va_start/va_arg hack).

On what basis does MSVC decide that UNREFERENCED_PARAMETER should
silence an unused parameter? IIRC, it expands to wrapping it's argument in
parenthesis. Could we teach clang to do the same? Sounds like it could be
a more general fix.

That sounds like a really good idea to me – teach sema that "(x);" has
the same meaning as "(void)x;".

Hold on a moment.

`(x);` certainly seems like it should silence an "unreferenced parameter"
warning (which is what this macro is for in MSVC), but it seems much less
reasonable for it to suppress an "unused value" warning (which is the
warning we're getting with Clang).

The reason it seems kind of reasonable to me is that adding parentheses
means "I really meant this" in a bunch of places. `if (a = b)` warns but
`if ((a = b))` doesn't, for example. So `(x);` seems more natural than
`(void)x;` to me. The latter admittedly does have going for it that it's an
established convention.

Can you think of an example where not warning on `(x);` would catch a bug?
Hm, I suppose many people like to always write parens after return, and
part of the motivation of -Wunused-value is catching forgetting "return"s.
So that's an argument for warning on `(x);`.

Perhaps I meant to write `f(x);` and wrote `(x);` instead, or my `x` is a
callable and I meant to write `x();`. Using parens to mean "I am discarding
this value" is novel, counter-intuitive, and just not a good pattern,
whereas the very purpose of permitting casts to void is to allow explicitly
discarding a value.

I'd really rather not do 2. Fighting with MSVC CRT headers over
va_start has been pretty awkward. Users have reported issues with our header
override fails. I'd rather not start fighting with winnt.h.

I'm OK with 3, but I'm desensitized to having to do this kind of
pattern matching for __uuidof, etc. Maybe we should wait for another +1. :slight_smile:

A +1 from me for option #3 as well. I'd feel more comfortable doing #2
if it was for correctness (along the lines of our va_start/va_arg hack).

On what basis does MSVC decide that UNREFERENCED_PARAMETER should
silence an unused parameter? IIRC, it expands to wrapping it's argument in
parenthesis. Could we teach clang to do the same? Sounds like it could be
a more general fix.

That sounds like a really good idea to me – teach sema that "(x);" has
the same meaning as "(void)x;".

Hold on a moment.

`(x);` certainly seems like it should silence an "unreferenced parameter"
warning (which is what this macro is for in MSVC), but it seems much less
reasonable for it to suppress an "unused value" warning (which is the
warning we're getting with Clang).

The reason it seems kind of reasonable to me is that adding parentheses
means "I really meant this" in a bunch of places. `if (a = b)` warns but `if
((a = b))` doesn't, for example. So `(x);` seems more natural than
`(void)x;` to me. The latter admittedly does have going for it that it's an
established convention.

Can you think of an example where not warning on `(x);` would catch a bug?
Hm, I suppose many people like to always write parens after return, and part
of the motivation of -Wunused-value is catching forgetting "return"s. So
that's an argument for warning on `(x);`.

Perhaps I meant to write `f(x);` and wrote `(x);` instead, or my `x` is a
callable and I meant to write `x();`. Using parens to mean "I am discarding
this value" is novel, counter-intuitive, and just not a good pattern,
whereas the very purpose of permitting casts to void is to allow explicitly
discarding a value.

Richard has a really compelling point; I think I am more in favor of
preventing the diagnostic only within an UNREFERENCED_PARAMETER macro
expansion. If we really find it's useful to use in a more broad
context, then we can allow it in the future without

~Aaron

It could also be a macro defined to just return the argument - i.e.
the same definition as UNREFERENCED_PARAMETER, but for a different
purpose. For example, /usr/include/xpc/base.h on OS X has:

#if __has_feature(objc_arc)
[..]
#define XPC_BRIDGEREF_BEGIN(xo) ((__bridge_retained void *)(xo))
[..]
#else // __has_feature(objc_arc)
[..]
#define XPC_BRIDGEREF_BEGIN(xo) (xo)
[..]
#endif // __has_feature(objc_arc)

One note, there is a GCC extension around for returning values from
blocks, so be careful with any change here.

Joerg