[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman closed this revision.
aaron.ballman added a comment.

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


https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-12 Thread S. B. Tam via Phabricator via cfe-commits
cpplearner added a comment.

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


https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-10 Thread S. B. Tam via Phabricator via cfe-commits
cpplearner updated this revision to Diff 141880.
cpplearner added a comment.

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


https://reviews.llvm.org/D45403

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/Sema/SemaDeclAttr.cpp
  test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp


Index: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
===
--- test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
+++ test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
@@ -2,7 +2,7 @@
 
 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 {
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -2042,16 +2042,6 @@
 static void handleUnusedAttr(Sema , Decl *D, const AttributeList ) {
   bool IsCXX17Attr = AL.isCXX11Attribute() && !AL.getScopeName();
 
-  if (IsCXX17Attr && isa(D)) {
-// The C++17 spelling of this attribute cannot be applied to a static data
-// member per [dcl.attr.unused]p2.
-if (cast(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)
Index: include/clang/Sema/AttributeList.h
===
--- include/clang/Sema/AttributeList.h
+++ include/clang/Sema/AttributeList.h
@@ -928,7 +928,6 @@
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
-  ExpectedForMaybeUnused,
 };
 
 } // namespace clang
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2834,8 +2834,7 @@
   "|types and namespaces"
   "|variables, functions and classes"
   "|kernel functions"
-  "|non-K functions"
-  "|variables, functions, methods, types, enumerations, enumerators, labels, 
and non-static data members}1">,
+  "|non-K functions}1">,
   InGroup;
 def err_attribute_wrong_decl_type : Error;
 def warn_type_attribute_wrong_type : Warning<


Index: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
===
--- test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
+++ test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
@@ -2,7 +2,7 @@
 
 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 {
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -2042,16 +2042,6 @@
 static void handleUnusedAttr(Sema , Decl *D, const AttributeList ) {
   bool IsCXX17Attr = AL.isCXX11Attribute() && !AL.getScopeName();
 
-  if (IsCXX17Attr && isa(D)) {
-// The C++17 spelling of this attribute cannot be applied to a static data
-// member per [dcl.attr.unused]p2.
-if (cast(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)
Index: include/clang/Sema/AttributeList.h
===
--- include/clang/Sema/AttributeList.h
+++ include/clang/Sema/AttributeList.h
@@ -928,7 +928,6 @@
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
-  ExpectedForMaybeUnused,
 };
 
 } // namespace clang
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2834,8 +2834,7 @@
   "|types and namespaces"
   "|variables, functions and classes"
   "|kernel functions"
-  "|non-K functions"
-  "|variables, functions, methods, types, enumerations, enumerators, 

[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-10 Thread S. B. Tam via Phabricator via cfe-commits
cpplearner added a comment.

In https://reviews.llvm.org/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").


Repository:
  rC Clang

https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-09 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:2054
-  }
-
   // If this is spelled as the standard C++17 attribute, but not in C++17, warn

Quuxplusone wrote:
> @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//. :)
> 
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.



Comment at: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp:5
   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]];
 };

Quuxplusone wrote:
> aaron.ballman wrote:
> > cpplearner wrote:
> > > lebedev.ri wrote:
> > > > 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.
> > > > ```
> > > 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.
> > 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?
> 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
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 "maybe it's unused, but shut up about it anyway." 
Definition vs declaration doesn't much matter in that case.


Repository:
  rC Clang

https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-09 Thread Arthur O'Dwyer via Phabricator via cfe-commits
Quuxplusone added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:2054
-  }
-
   // If this is spelled as the standard C++17 attribute, but not in C++17, warn

@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//. :)




Comment at: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp:5
   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]];
 };

aaron.ballman wrote:
> cpplearner wrote:
> > lebedev.ri wrote:
> > > 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.
> > > ```
> > 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.
> 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?
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


Repository:
  rC Clang

https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-09 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment.

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?




Comment at: lib/Sema/SemaDeclAttr.cpp:2050
-  S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
-  << AL.getName() << ExpectedForMaybeUnused;
-  return;

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



Comment at: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp:5
   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]];
 };

cpplearner wrote:
> lebedev.ri wrote:
> > 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.
> > ```
> 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.
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?


Repository:
  rC Clang

https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-07 Thread S. B. Tam via Phabricator via cfe-commits
cpplearner added inline comments.



Comment at: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp:5
   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]];
 };

lebedev.ri wrote:
> 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.
> ```
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.


Repository:
  rC Clang

https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-07 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added inline comments.



Comment at: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp:5
   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]];
 };

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


Repository:
  rC Clang

https://reviews.llvm.org/D45403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D45403: Make [[maybe_unused]] work with static data members

2018-04-07 Thread S. B. Tam via Phabricator via cfe-commits
cpplearner created this revision.
cpplearner added reviewers: aaron.ballman, rsmith.
Herald added a subscriber: cfe-commits.

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


Repository:
  rC Clang

https://reviews.llvm.org/D45403

Files:
  lib/Sema/SemaDeclAttr.cpp
  test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp


Index: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
===
--- test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
+++ test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
@@ -2,7 +2,7 @@
 
 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 {
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -2042,16 +2042,6 @@
 static void handleUnusedAttr(Sema , Decl *D, const AttributeList ) {
   bool IsCXX17Attr = AL.isCXX11Attribute() && !AL.getScopeName();
 
-  if (IsCXX17Attr && isa(D)) {
-// The C++17 spelling of this attribute cannot be applied to a static data
-// member per [dcl.attr.unused]p2.
-if (cast(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)


Index: test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
===
--- test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
+++ test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p2.cpp
@@ -2,7 +2,7 @@
 
 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 {
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -2042,16 +2042,6 @@
 static void handleUnusedAttr(Sema , Decl *D, const AttributeList ) {
   bool IsCXX17Attr = AL.isCXX11Attribute() && !AL.getScopeName();
 
-  if (IsCXX17Attr && isa(D)) {
-// The C++17 spelling of this attribute cannot be applied to a static data
-// member per [dcl.attr.unused]p2.
-if (cast(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)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits