Page MenuHomePhabricator

Make [[maybe_unused]] work with static data members
ClosedPublic

Authored by cpplearner on Apr 7 2018, 3:42 AM.

Details

Summary

IIUC a static data member is a variable, so [[maybe_unused]] should be allowed to apply to a static data member.

Diff Detail

Event Timeline

cpplearner created this revision.Apr 7 2018, 3:42 AM
lebedev.ri added inline comments.
test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
5

As the code comment noted, http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf, page 199:

2 The attribute may be applied to the declaration of a class, a typedef-name, a variable, a **non-static** data
member, a function, an enumeration, or an enumerator.
cpplearner added inline comments.Apr 7 2018, 4:23 AM
test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
5

That section says that [[maybe_unused]] can be applied to a non-static data member, which doesn't mean it can't be applied to a static data member.

And I'm arguing that since a static data member is a variable, [dcl.attr.unused]p2 actually allows [[maybe_unused]] to be applied to a static data member.

I don't think we're currently diagnosing static data members of classes as being unused in the first place; are there plans to implement that functionality so that someone might want to write the attribute there?

lib/Sema/SemaDeclAttr.cpp
2050

ExpectedForMaybeUnused can be removed from AttributeList.h and the warn_attribute_wrong_decl_type table definition can have the entry for ExpectedForMaybeUnused removed as well.

test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
5

Yes -- through twisty standardese, a static data member of a class might be a variable. This test case, however, is only a declaration and not a definition (which means it's not an object, which means it's not a variable). Should the attribute still appertain in this case?

Quuxplusone added inline comments.
lib/Sema/SemaDeclAttr.cpp
2054

@aaron.ballman writes:

I don't think we're currently diagnosing static data members of classes as being unused in the first place; are there plans to implement that functionality so that someone might want to write the attribute there?

FWIW, "plans to implement that functionality" (in Clang) is not the only reason to accept the attribute. Someone might write the attribute in their codebase that currently compiles with some other compiler which implements the functionality (or at least does not reject it); it would be mildly unfortunate if that made their codebase non-portable-to-Clang. (Only "mildly" because the diagnostic being removed here is just a warning; the user could suppress it if they really needed to.)

Here's an example of code that compiles cleanly with GCC but gives an arguably "false positive" diagnostic when compiled with Clang.
https://wandbox.org/permlink/UG4kG5XTBn12xfuu
Now, admittedly, both GCC and Clang produce a "false negative" re the unused private static member y; but that false negative might get fixed in the future. The user writes their code today and it must compile today. :)

test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
5

The attribute does currently apply to declarations as well as definitions, although you have to be a real language lawyer to observe it.
https://wandbox.org/permlink/WBLWBdd42rv95UaS

aaron.ballman added inline comments.Apr 9 2018, 3:22 PM
lib/Sema/SemaDeclAttr.cpp
2054

Agreed; that's why the analysis isn't a requirement for accepting this patch. I was mostly just curious if there were plans to extend that functionality or not.

test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
5

Good point. Also, declaration vs definition doesn't really matter when I think about it -- the whole point is to tell you "hey, this is unused" and give the programmer a way to say "maybbbbbe it's unused, but shut up about it anyway." Definition vs declaration doesn't much matter in that case.

I don't think we're currently diagnosing static data members of classes as being unused in the first place; are there plans to implement that functionality so that someone might want to write the attribute there?

Currently, unused static data members are diagnosed when the enclosing class is in an anonymous namespace: https://wandbox.org/permlink/50nTcaESHdjK8pkd (ironically, the message is "unused variable").

Removed ExpectedForMaybeUnused from AttributeList.h, and removed the entry for ExpectedForMaybeUnused from the warn_attribute_wrong_decl_type table definition.

cpplearner marked an inline comment as done.Apr 10 2018, 11:36 AM
This revision is now accepted and ready to land.Apr 10 2018, 12:08 PM

I do not have commit access. Could someone commit the change for me?

aaron.ballman closed this revision.Apr 12 2018, 5:24 AM

I've commit in r329904, thank you for the patch!