[NFC][CodingStandard] Require[[maybe_unused]] for unused variables in asserts

[jurahul]

Require using attribute [[maybe_unused] for assert-only variables that may be unused in non-assert enabled builds to suppress unused variable warnings.

[fmayer]

Just confirming that this means something like

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]

[jurahul]

Just confirming that this means something like

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]

Yes. The attribute is needed only in function definition and not in the declaration.

[fmayer]

Yes. The attribute is needed only in function definition and not in the declaration.

My point being that we lose some precision about where it is unused, but that we think that is ok.

[jurahul]

I see your point. If due to a bug it became unused in the #else path the prior code would still warn, the new one would not. I'll let the reviewers decide if this loss of precision is fine. If we really need to preserve this precision while still using maybe_unused it seems we could do something like:

void f(int x) {
#ifdef XYZ
  [[maybe_unused]] int dummy = x;
#else
  // use x;
#endif
}      

[chrisjbris]

Is it worth mentioning that [[maybe_unused]] is also applicable to functions too, not just variables?

[jh7370]

This doesn't clearly explain when [[maybe_unused]] is not acceptable, so it isn't really a useful standard, in my opinion.

[jurahul]

Yeah, this was an attempt to keep the complex cases (per the discourse discussion) unspecified. Maybe we can just say that for variables uses solely in asserts, you must use [[maybe_unused]] and remove the simple cases language. That is say:

• To suppress unused variable warnings for variables used solely in asserts, use [[maybe_unused]].
• For other cases, either [[maybe_unused]] or C-style void cast is acceptable.

[jh7370]

That's an improvement, certainly, though I should add that I'm an "always [[maybe_unused]] proponent, since it more clearly expresses intent to those not as experienced with programming in general.

[jurahul]

Right, but based on the forum discussion, not everyone is on board with that. I have amended the PR now with the above changes. The delta now is essentially that for variables used only in asserts, we require the use of maybe_unused over C-style casts. I am hoping that will be an acceptable delta.

[jh7370]

I'm happy with this, but please wait for others.

[nikic]

I'm ok with it.

[jurahul]

Thanks. Will wait for other folks to review as well.

[chrisjbris]

lgtm

[jurahul]

Thanks. @nikic and/or @AaronBallman if one of you can approve as well, I will go ahead and merge it. Thanks!

[AaronBallman]

LGTM!