Make [[maybe_unused]] work with static data members

[cpplearner]

Summary

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

[lebedev.ri]

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]

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.

[aaron.ballman]

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]

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]

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.

[cpplearner]

In D45403#1062276, @aaron.ballman wrote:

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

[cpplearner]

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

[aaron.ballman]

LGTM!

[cpplearner]

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

[aaron.ballman]

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

include/clang/Basic/DiagnosticSemaKinds.td

def warn_attribute_wrong_decl_type : Warning<
   "|functions, methods and blocks"
   "|functions, methods, and parameters"
   "|variables"
   "|variables and fields"
   "|variables, data members and tag types"
   "|types and namespaces"
   "|variables, functions and classes"
   "|kernel functions"
-  "|non-K&R-style functions"
-  "|variables, functions, methods, types, enumerations, enumerators, labels, and non-static data members}1">,
+  "|non-K&R-style functions}1">,
   InGroup<IgnoredAttributes>;
 def err_attribute_wrong_decl_type : Error<warn_attribute_wrong_decl_type.Text>;
 def warn_type_attribute_wrong_type : Warning<
   "'%0' only applies to %select{function|pointer|"
   "Objective-C object or block pointer}1 types; type here is %2">,
   InGroup<IgnoredAttributes>;
 def warn_incomplete_encoded_type : Warning<
   "encoding of %0 type is incomplete because %1 component has unknown encoding">,

include/clang/Sema/AttributeList.h

enum AttributeDeclKind {
   ExpectedFunctionMethodOrParameter,
   ExpectedVariable,
   ExpectedVariableOrField,
   ExpectedVariableFieldOrTag,
   ExpectedTypeOrNamespace,
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
-  ExpectedForMaybeUnused,
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_SEMA_ATTRIBUTELIST_H

lib/Sema/SemaDeclAttr.cpp

static void handleDependencyAttr(Sema &S, Scope *Scope, Decl *D,
   D->addAttr(::new (S.Context) CarriesDependencyAttr(
                                    AL.getRange(), S.Context,
                                    AL.getAttributeSpellingListIndex()));
 }
 
 static void handleUnusedAttr(Sema &S, Decl *D, const AttributeList &AL) {
   bool IsCXX17Attr = AL.isCXX11Attribute() && !AL.getScopeName();
 
-  if (IsCXX17Attr && isa<VarDecl>(D)) {
-    // The C++17 spelling of this attribute cannot be applied to a static data
-    // member per [dcl.attr.unused]p2.
-    if (cast<VarDecl>(D)->isStaticDataMember()) {
-      S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
-          << AL.getName() << ExpectedForMaybeUnused;
-      return;
-    }
-  }
-
   // If this is spelled as the standard C++17 attribute, but not in C++17, warn
   // about using it as an extension.
   if (!S.getLangOpts().CPlusPlus17 && IsCXX17Attr)
     S.Diag(AL.getLoc(), diag::ext_cxx17_attr) << AL.getName();
 
   D->addAttr(::new (S.Context) UnusedAttr(
       AL.getRange(), S.Context, AL.getAttributeSpellingListIndex()));
 }

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

 // RUN: %clang_cc1 -fsyntax-only -Wunused -std=c++1z -verify %s
 
 struct [[maybe_unused]] S {
   int I [[maybe_unused]];
-  static int SI [[maybe_unused]]; // expected-warning {{'maybe_unused' attribute only applies to variables, functions, methods, types, enumerations, enumerators, labels, and non-static data members}}
+  static int SI [[maybe_unused]];
 };
 
 enum [[maybe_unused]] E1 {
   EnumVal [[maybe_unused]]
 };
 
 [[maybe_unused]] void unused_func([[maybe_unused]] int parm) {
   typedef int maybe_unused_int [[maybe_unused]];
   [[maybe_unused]] int I;
 }
 
 namespace [[maybe_unused]] N {} // expected-warning {{'maybe_unused' attribute only applies to}}