Coding standard: Deprecate use of void cast to suppress unused variable warnings?

[danilaml]

You can tweak the query all you want, but I would not trust what it spits out at all, as already seen in this thread. It’s not a reliable source of information.

[jh7370]

I’m pretty sure I see unused parameter warnings in Visual Studio builds routinely, with warning levels cranked up.

[jurahul]

I did the unused params experiment as well in compiler explorer and only Visual Studio warned about it, neither clang nor gcc did

[kuhar]

FYI, there was a recent-ish thread that touched the same topic: [[maybe-unused]] vs __attribute__((unused)) vs (void) var for assert variables

Some handpicked replies:

[[maybe-unused]] vs __attribute__((unused)) vs (void) var for assert variables

I don’t think this is an area where we should try to innovate. Casting to void is a tried and true idiom for indicating that a value is unused. It’s even documented in the coding standards for folks who aren’t familiar with the idiom. I don’t see value in deviating from C just for it’s own sake, there should be some value proposition, like safety, for doing so.

[[maybe-unused]] vs __attribute__((unused)) vs (void) var for assert variables

Just MHO I don’t see an advantage to using the attribute. It is more verbose and the cast to void idiom is obvious enough. The one advantage I see of the attribute is that it is more “explicit”, but this is C++ which is full of magic and special cases (sadly). I don’t think this is better than the old C idiom.

[[maybe-unused]] vs __attribute__((unused)) vs (void) var for assert variables

The advantage of [[maybe_unused]] attribute is that it can apply to pretty much anything. I.e. you can mark functions/arguments with it, if there is some rarely used value (like for printf-like functions). But for simple unused variables, (void) is more concise idiom understood by pretty much everyone.

[[maybe-unused]] vs __attribute__((unused)) vs (void) var for assert variables

In the past, “don’t mass-convert” was generally applied to more extensive changes such as clang-formatting entire files or renaming methods/fields to conform to newer spelling recommendations. The latter sorts of changes are commonly held to interfere with other people’s work on the same files. (Although, clang-formatting a file prior to doing extensive work on that file is actually seen as a good thing, because the individual changes are expected to be clang-formatted.) In this case, however, …

[[maybe-unused]] vs __attribute__((unused)) vs (void) var for assert variables

Consistency of the codebase matters a lot to me.

[kparzysz]

That was two years ago. I don’t think it hurts to revisit this topic given that most responses here are non-negative.

[jurahul]

For #1, the declaration of func will not have the [[maybe_unused]], so in that sense that implementation detail is still hidden and stays with the implementation.

[jurahul]

Maybe we can recommend [[maybe_unused]] when all paths in the code need this attribute, whereas if there are only some paths that need it, go with void cast to maintain precision? So, in the common case of variables used in assert only, we will use [[maybe_unused]] (the example code snippet in the coding standard doc).

I also want to mention a related case: debug only print code which can also create its own local variables. There, in our downstream code, we have adopted the convention to wrap all variable definitions and debug only code within LLVM_DEBUG macro as such:

   LLVM_DEBUG({
        auto I = Table.find(x);
        dbgs() <<  ... (code using I)
   });

But if that’s not possible in some cases, we need to resort to either [[maybe_unused]] or void cast in these cases as well.

[AaronBallman]

@jurahul

For #1, the declaration of func will not have the [[maybe_unused]], so in that sense that implementation detail is still hidden and stays with the implementation.

That’s not always true though – we have plenty of helper functions with no separate declaration/definition.

[chrisjbris]

I remain in favour of this change. I think one of the strong arguments for the coding standard update (if not the codebase) is the wider versatility of [[maybe_unused]], as it can be applied to functions too.

[ssahasra]

Thank you for pulling these out! The fact that casting to void is a well-recognized idiom totally resonates with me. I don’t really see the value of moving to the attribute here.

Here’s an example currently being discussed in the review:

void f(int x) {
#ifdef XYZ
  (void)x;
  [...]
#else
  [do something with x]

becomes

void f([[maybe_unused]] int x) {
#ifdef XYZ
  [...]
#else
  [do something with x]

The cast-to-void in this case is much clearer to me. This is independent of whether int x is a function parameter.

[bogner]

@ssahasra

Here’s an example currently being discussed in the review:

void f(int x) {
#ifdef XYZ
  (void)x;
  [...]
#else
  [do something with x]

becomes

void f([[maybe_unused]] int x) {
#ifdef XYZ
  [...]
#else
  [do something with x]

If I were to see either version of this example in code review I would probably ask for it to be refactored. I really don’t think we should be making any recommendation on what to do in complicated cases like this in a note about casting.

I suspect the right thing to do is to say that it’s acceptable to use a (void) cast to suppress warnings about variables that are only used in an assert, but in simple cases we should prefer [[maybe_unused]] anyway.

Notably, the existing advice is really only about cases like this:

int x = foo();
(void)x;
assert(someCondition(x));

and in this case, the alternative is clearly simpler:

[[maybe_unused]] int x = foo();
assert(someCondition(x));

This is more concise, avoids issues with code motion where the void cast drifts away from the definition, and easy to understand even by someone who happens to be unfamiliar with the idiom.

[jurahul]

Thanks all again for the discussion. Based on the feedback so far, I have updated the PR I have to

1. Keep allowing C style void casting (but mentioning that [[maybe_unsed]] is preferred over void cast in simple cases)
2. Updated the example code for assert to use [[maybe_unused]]
3. Added a C example where we have to use void casting

[NFC][CodingStandard] Prefer use of [[maybe_unused]] over C-style void cast for unused variables by jurahul · Pull Request #142850 · llvm/llvm-project

@bogner @chrisjbris @jh7370 and other PTAL and see if this seems like a reasonable amendment to the coding standard.

[MaskRay]

void f([[maybe_unused]] int x) {

We should not use [[maybe_unused]] on function parameters. Unused parameters and used heavily in the code base and llvm/cmake/modules/HandleLLVMOptions.cmake specifies -Wno-unused-parameter.

[arsenm]

@MaskRay

Unused parameters and used heavily in the code base and llvm/cmake/modules/HandleLLVMOptions.cmake specifies -Wno-unused-parameter.

Would be a lot better if we didn’t do this, we should stop using -Wno-unused-parameter

[jurahul]

I have a 3rd version on the PR now, with a hopefully acceptable middle ground here. We just say that for variables that are unused because they are used only in asserts, we must use [[maybe_unused]]. That leaves other “complex” cases unspecified, like unused function parameters or variables used only in debug code, in the sense either [[maybe_unused]] or C-style void case would be acceptable in those cases (i..e, no change in coding standard for these).

[jurahul]

For unused function parameters, it seems we should distinguish between cases where a parameter is truly unreferenced anywhere in the code vs conditionally referenced. In the former case, I think we can just not name the parameter and that will avoid any warnings. That is:

void foo(int x, int /*unused*/) {
}

This can happen if the function is implementing an interface that requires it to have a specific signature, so we cannot just drop the unreferenced param.

[jurahul]

The PR above has been merged now. Thanks again to folks who participated in the discussion.