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

[fhahn]

@jh7370

Personally, I think I’m now leaning towards preferring [[maybe_unused]] for clarity (and also it allows avoiding the exception I needed to add to the coding standard in the casting rules).

Personally I also think [[maybe_unused]] is preferable due to clarity

[chrisjbris]

If we’re happy to change this I can write a script to perform the replacements and generate diffs in chunks of 10-20 files and start pushing up diffs for review. I’ll see if I can get an idea of how many files would need to be changed before I start in ernest.

My grep & regex skills are poor, so please help me correct the following if you can. Certainly the type list probably has a nicer experssion, at the very least. Using the rudementary grep below in the llvm-project root:

pcregrep -M -rl --include='\.cpp$' --include='\.c$' --include='\.h$'    '(uint[0-9]+t|int|char|float|double|bool|unsigned)[ ]([a-zA-Z]+)[ ].*\n.*\(void\)[ ]*\g{-1}' ../ | wc -l

I get 183 source files as candidates for modification.

[kuhar]

@chrisjbris

My grep & regex skills are poor, so please help me correct the following if you can.

This would be great to have as a clang-tidy check.

[s-barannikov]

I’m not sure why this is being discussed at all.
Just because we’re using C++17 does not mean we should abuse it.

[chrisjbris]

Hi @s-barannikov, I’m happy to hear reasons against using the attribute and why it constitutes abuse. After all, the current idiom does function.

However, the discussion above seems to indicate a desire to move to the attribute.

[s-barannikov]

The reasons have already been spelled by others, I have nothing to add.
The main point is that (void) is a well recognized idiom.
[[maybe_unused]] must have a significant advantage for it to be considered.

@chrisjbris

However, the discussion above seems to indicate a desire to move to the attribute.

The number of hearts each post received indicates otherwise, if I count correctly.

[kparzysz-quic]

@s-barannikov

[[maybe_unused]] must have a significant advantage for it to be considered.

There you go:

@danilaml

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).

If we want consistency, then the attribute is the way to go.

[s-barannikov]

I don’t mind using it on functions. I think nobody would oppose that, because the alternative is LLVM_ATTRIBUTE_UNUSED.

When it comes to variables, this becomes more sensitive, because there are many more of them. There couldn’t be more low level component of a product than a coding style. Not only the code is already written, but the habbits are developed. The Big Switch can be carried out, but I can imagine people silently asking “But why? I’m so used to it.”.

@kparzysz-quic

@danilaml

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).

If we want consistency, then the attribute is the way to go.

There is no contradiction, we can consistently use it on functions. An analogy is the ‘const’ qualifier. We use it, but we’re not obsessed with it.

So far I’ve seen justifications for using the attribute, but not arguments for switching to it.
I won’t object if I see the use of the attribute in a review, but I don’t want to be forced to use it.
(The same applies to the other RFC that proposes switching to C++-style casts.)

[s-barannikov]

[offtopic]
Is there a way to organize a poll to eliminate the “active minority” factor on either side?
Does discourse allow it, e.g. by sending a notification? This would convince me.

[mehdi_amini]

@s-barannikov

I won’t object if I see the use of the attribute in a review, but I don’t want to be forced to use it.

Consistency of the codebase matters a lot to me.

[s-barannikov]

@mehdi_amini

Consistency of the codebase matters a lot to me.

To me, too. But is this particular case that important?
There are several different styles for function argument comments in the codebase:

1. /*name=*/value
2. /*name*/ value
3. /* name */value
4. value/*name*/

I personally prefer (1). Just becase my IDE can insert it automatically. But I’m used to it and wouldn’t want to be forced to use any other style. I consider (void) vs [[maybe_unused]] the same kind of issue.

[mehdi_amini]

@s-barannikov

But is this particular case that important?

Just as much as almost anything else that is in the coding guidelines?

@s-barannikov

There are several different styles for function argument comments in the codebase:

Actually we have a clang-tidy check that relies on 1 to check consistency between caller and caller (so this is more than a style issue) and I am quite sure I ask for this format in code review.
If we don’t document it I would definitely look into doing it.
There is a lot inconsistency from history in the codebase, but that’s not a good excuse to avoid standardizing the style guide IMO

[s-barannikov]

@mehdi_amini

Just as much as almost anything else that is in the coding guidelines?

It is not in the coding guidelines (yet). If it is standardized, I’ll be obliged to use it and won’t complain.

Actually we have a clang-tidy check to enforce 1, and I am quite sure I ask for this format in code review.

Good to know, thanks!

There is a lot inconsistency from history in the codebase, but that’s not a good excuse to avoid standardizing the style guide IMO

I agree. However, in order to standardize it, there should be a consensus. I’m afraid there will not be one, there is just not enough data to say which option wins. Looks like I’m kind of safe for now

[tschuett]

@mehdi_amini

Consistency of the codebase matters a lot to me.

Then you should have a chat with the Flang people.

[mehdi_amini]

If you have specific things about flang where it does not follow the style guide we can discuss it (in another place than this thread), as far as I remember the agreement to merge flang in LLVM was that it would adhere to the LLVM coding standard.

[tschuett]

“” Always wrap the bodies of if() , else , while() , for() , do , &c. with braces, even when the body is a single statement or empty. The opening { goes on the end of the line, not on the next line. Functions also put the opening { after the formal arguments or new-style result type, not on the next line. Use {} for empty inline constructors and destructors in classes.“”