[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367027: Implement P1771 (authored by erichkeane, committed 
by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64914?vs=211740=211761#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/lib/Sema/SemaStmt.cpp
  cfe/trunk/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
  cfe/trunk/test/Preprocessor/has_attribute.cpp
  cfe/trunk/www/cxx_status.html

Index: cfe/trunk/lib/AST/Expr.cpp
===
--- cfe/trunk/lib/AST/Expr.cpp
+++ cfe/trunk/lib/AST/Expr.cpp
@@ -2563,13 +2563,31 @@
   case CXXTemporaryObjectExprClass:
   case CXXConstructExprClass: {
 if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
-  if (Type->hasAttr()) {
+  const auto *WarnURAttr = Type->getAttr();
+  if (Type->hasAttr() ||
+  (WarnURAttr && WarnURAttr->IsCXX11NoDiscard())) {
 WarnE = this;
 Loc = getBeginLoc();
 R1 = getSourceRange();
 return true;
   }
 }
+
+const auto *CE = cast(this);
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  const auto *WarnURAttr = Ctor->getAttr();
+  if (WarnURAttr && WarnURAttr->IsCXX11NoDiscard()) {
+WarnE = this;
+Loc = getBeginLoc();
+R1 = getSourceRange();
+
+if (unsigned NumArgs = CE->getNumArgs())
+  R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
+   CE->getArg(NumArgs - 1)->getEndLoc());
+return true;
+  }
+}
+
 return false;
   }
 
Index: cfe/trunk/lib/Sema/SemaStmt.cpp
===
--- cfe/trunk/lib/Sema/SemaStmt.cpp
+++ cfe/trunk/lib/Sema/SemaStmt.cpp
@@ -196,6 +196,25 @@
   return true;
 }
 
+static bool DiagnoseNoDiscard(Sema , const WarnUnusedResultAttr *A,
+  SourceLocation Loc, SourceRange R1,
+  SourceRange R2, bool IsCtor) {
+  if (!A)
+return false;
+  StringRef Msg = A->getMessage();
+
+  if (Msg.empty()) {
+if (IsCtor)
+  return S.Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return S.Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
+  }
+
+  if (IsCtor)
+return S.Diag(Loc, diag::warn_unused_constructor_msg) << A << Msg << R1
+  << R2;
+  return S.Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
+}
+
 void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
   if (const LabelStmt *Label = dyn_cast_or_null(S))
 return DiagnoseUnusedExprResult(Label->getSubStmt());
@@ -254,19 +273,19 @@
 return;
 
   E = WarnExpr;
+  if (const auto *Cast = dyn_cast(E))
+if (Cast->getCastKind() == CK_NoOp ||
+Cast->getCastKind() == CK_ConstructorConversion)
+  E = Cast->getSubExpr()->IgnoreImpCasts();
+
   if (const CallExpr *CE = dyn_cast(E)) {
 if (E->getType()->isVoidType())
   return;
 
-if (const auto *A = cast_or_null(
-CE->getUnusedResultAttr(Context))) {
-  StringRef Msg = A->getMessage();
-  if (!Msg.empty())
-Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
-  else
-Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
+if (DiagnoseNoDiscard(*this, cast_or_null(
+ CE->getUnusedResultAttr(Context)),
+  Loc, R1, R2, /*isCtor=*/false))
   return;
-}
 
 // If the callee has attribute pure, const, or warn_unused_result, warn with
 // a more specific message to make it clear what is happening. If the call
@@ -284,9 +303,24 @@
 return;
   }
 }
+  } else if (const auto *CE = dyn_cast(E)) {
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  const auto *A = Ctor->getAttr();
+  A = A ? A : Ctor->getParent()->getAttr();
+  if (DiagnoseNoDiscard(*this, A, Loc, R1, R2, /*isCtor=*/true))
+return;
+}
+  } else if (const auto *ILE = dyn_cast(E)) {
+if (const TagDecl *TD = ILE->getType()->getAsTagDecl()) {
+
+  if (DiagnoseNoDiscard(*this, TD->getAttr(), Loc, R1,
+R2, /*isCtor=*/false))
+return;
+}
   } else if (ShouldSuppress)
 return;
 
+  E = WarnExpr;
   if (const ObjCMessageExpr *ME = dyn_cast(E)) {
 if (getLangOpts().ObjCAutoRefCount && ME->isDelegateInitCall()) {
   Diag(Loc, diag::err_arc_unused_init_message) << R1;
@@ -294,14 +328,9 @@
 }
 const ObjCMethodDecl *MD = 

[PATCH] D64914: Implement P1771

[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 aside from a minor naming nit.




Comment at: clang/lib/Sema/SemaStmt.cpp:201
+  SourceLocation Loc, SourceRange R1,
+  SourceRange R2, bool isCtor) {
+  if (!A)

`isCtor` -> `IsCtor`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 211740.
erichkeane marked 2 inline comments as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -664,7 +664,7 @@
 

 http://wg21.link/p1771r1;>P1771R1 (DR)
-No
+SVN
   
 
   [[maybe_unused]] attribute
Index: clang/test/Preprocessor/has_attribute.cpp
===
--- clang/test/Preprocessor/has_attribute.cpp
+++ clang/test/Preprocessor/has_attribute.cpp
@@ -63,7 +63,7 @@
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
 // WINDOWS: no_unique_address: 0
-// CHECK: nodiscard: 201603L
+// CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // FIXME(201803L) CHECK: unlikely: 0
 
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -80,6 +80,50 @@
   conflicting_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: special reason}}
 }
 
+namespace p1771 {
+struct[[nodiscard("Don't throw me away!")]] ConvertTo{};
+struct S {
+  [[nodiscard]] S();
+  [[nodiscard("Don't let that S-Char go!")]] S(char);
+  S(int);
+  [[gnu::warn_unused_result]] S(double);
+  operator ConvertTo();
+  [[nodiscard]] operator int();
+  [[nodiscard("Don't throw away as a double")]] operator double();
+};
+
+struct[[nodiscard("Don't throw me away either!")]] Y{};
+
+void usage() {
+  S();// expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+  S('A'); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+  S(1);
+  S(2.2);
+  Y(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away either!}}
+  S s;
+  ConvertTo{}; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+
+// AST is different in C++20 mode, pre-2017 a move ctor for ConvertTo is there
+// as well, hense the constructor warning.
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  (ConvertTo) s;
+  (int)s; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  (S)'c'; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  static_cast(s);
+  static_cast(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  static_cast(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw away as a double}}
+}
+}; // namespace p1771
+
 #ifdef EXT
 // expected-warning@5 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@9 {{use of the 'nodiscard' attribute is a C++17 extension}}
@@ -91,4 +135,10 @@
 // expected-warning@71 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@73 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@74 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@84 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@86 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@87 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@91 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@92 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@95 {{use of the 'nodiscard' attribute is a C++2a extension}}
 #endif
Index: clang/lib/Sema/SemaStmt.cpp

[PATCH] D64914: Implement P1771

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:296-301
+  if (A) {
+StringRef Msg = A->getMessage();
+if (!Msg.empty())
+  Diag(Loc, diag::warn_unused_constructor_msg) << A << Msg << R1 << R2;
+else
+  Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;

We use this pattern in a handful of places. I wonder if we want to add a 
private function to `Sema` to handle this: `void DiagnoseNodiscard(const 
WarnUnusedResultAttr *A, SourceLocation Loc, bool IsConstructor);` (maybe it 
could return a partial diagnostic so we can optionally apply the source ranges 
to it as well?)



Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:91
+  operator ConvertTo();
+  [[nodiscard]] operator int();
+};

Can you add a test that adds a message to the `nodiscard` attribute written 
directly on a conversion operator?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 211504.
erichkeane added a comment.

Casting implementation had to happen in SemaStmt, because other 'warn unused 
result' stuff depends on them being the way they are.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -664,7 +664,7 @@
 

 http://wg21.link/p1771r1;>P1771R1 (DR)
-No
+SVN
   
 
   [[maybe_unused]] attribute
Index: clang/test/Preprocessor/has_attribute.cpp
===
--- clang/test/Preprocessor/has_attribute.cpp
+++ clang/test/Preprocessor/has_attribute.cpp
@@ -63,7 +63,7 @@
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
 // WINDOWS: no_unique_address: 0
-// CHECK: nodiscard: 201603L
+// CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // FIXME(201803L) CHECK: unlikely: 0
 
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -80,6 +80,48 @@
   conflicting_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: special reason}}
 }
 
+namespace p1771 {
+struct[[nodiscard("Don't throw me away!")]] ConvertTo{};
+struct S {
+  [[nodiscard]] S();
+  [[nodiscard("Don't let that S-Char go!")]] S(char);
+  S(int);
+  [[gnu::warn_unused_result]] S(double);
+  operator ConvertTo();
+  [[nodiscard]] operator int();
+};
+
+struct[[nodiscard("Don't throw me away either!")]] Y{};
+
+void usage() {
+  S();// expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+  S('A'); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+  S(1);
+  S(2.2);
+  Y(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away either!}}
+  S s;
+  ConvertTo{}; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+
+// AST is different in C++20 mode, pre-2017 a move ctor for ConvertTo is there
+// as well, hense the constructor warning.
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  (ConvertTo) s;
+  (int)s; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  (S)'c'; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  static_cast(s);
+  static_cast(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+}
+}; // namespace p1771
+
 #ifdef EXT
 // expected-warning@5 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@9 {{use of the 'nodiscard' attribute is a C++17 extension}}
@@ -91,4 +133,9 @@
 // expected-warning@71 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@73 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@74 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@84 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@86 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@87 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@91 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@94 {{use of the 'nodiscard' attribute is a C++2a extension}}
 #endif
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -254,6 +254,11 @@
 return;
 
   E = WarnExpr;
+  if 

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Disregard that last commit... i ended up breaking stuff apparently.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 211482.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -664,7 +664,7 @@
 

 http://wg21.link/p1771r1;>P1771R1 (DR)
-No
+SVN
   
 
   [[maybe_unused]] attribute
Index: clang/test/Preprocessor/has_attribute.cpp
===
--- clang/test/Preprocessor/has_attribute.cpp
+++ clang/test/Preprocessor/has_attribute.cpp
@@ -63,7 +63,7 @@
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
 // WINDOWS: no_unique_address: 0
-// CHECK: nodiscard: 201603L
+// CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // FIXME(201803L) CHECK: unlikely: 0
 
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -80,6 +80,48 @@
   conflicting_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: special reason}}
 }
 
+namespace p1771 {
+struct[[nodiscard("Don't throw me away!")]] ConvertTo{};
+struct S {
+  [[nodiscard]] S();
+  [[nodiscard("Don't let that S-Char go!")]] S(char);
+  S(int);
+  [[gnu::warn_unused_result]] S(double);
+  operator ConvertTo();
+  [[nodiscard]] operator int();
+};
+
+struct[[nodiscard("Don't throw me away either!")]] Y{};
+
+void usage() {
+  S();// expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+  S('A'); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+  S(1);
+  S(2.2);
+  Y(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away either!}}
+  S s;
+  ConvertTo{}; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+
+// AST is different in C++20 mode, pre-2017 a move ctor for ConvertTo is there
+// as well, hense the constructor warning.
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  (ConvertTo) s;
+  (int)s; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  (S)'c'; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  static_cast(s);
+  static_cast(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+}
+}; // namespace p1771
+
 #ifdef EXT
 // expected-warning@5 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@9 {{use of the 'nodiscard' attribute is a C++17 extension}}
@@ -91,4 +133,9 @@
 // expected-warning@71 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@73 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@74 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@84 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@86 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@87 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@91 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@94 {{use of the 'nodiscard' attribute is a C++2a extension}}
 #endif
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -284,6 +284,30 @@
 return;
   }
 }
+  } else if (const auto *CE = dyn_cast(E)) {
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  const auto *A = Ctor->getAttr();
+  A = A ? 

[PATCH] D64914: Implement P1771

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2835
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {
 S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;

erichkeane wrote:
> aaron.ballman wrote:
> > Conversion functions?
> Conversion functions don't return 'void', so the 2nd condition should do it, 
> right?
true -- we already seem to diagnose a conversion function that returns void 
anyway. Sorry for the noise!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane added a comment.

Ugg... well conversion functions are an interesting bit, as well as Type{}; 
with no constructors.  It ends up being a function style cast to an 
init-list-expr, so its going to require a bit more work.  Stay tuned :)




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2835
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {
 S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;

aaron.ballman wrote:
> Conversion functions?
Conversion functions don't return 'void', so the 2nd condition should do it, 
right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D64914#1595660 , @erichkeane wrote:

> Rebased and did all the comments (including the www_status).  @aaron.ballman 
> : Wasn't positive what you meant about the conversion functions, but I think 
> I got one?


I was talking about [dcl.attr.nodiscard]p2.2: "an explicit type conversion 
(7.6.1.8 [expr.static.cast], 7.6.3 [expr.cast], 7.6.1.3 [expr.type.conv]) that 
constructs an object through a constructor declared nodiscard, or that 
initializes an object of a nodiscard type." and specifically the part about 
type conversion operators that are marked `nodiscard` directly.




Comment at: clang/include/clang/Basic/AttrDocs.td:1518
+marked_ctor(); // diagnoses.
+marked_ctor(3); // Does not diagnose, int constructor isn't marked 
nodiscard.
+  }

I think we may want to add a case like:
```
struct S {
  operator marked_type() const;
  [[nodiscard]] operator int() const;
};

void func() {
  S s;
  static_cast(s); // diagnoses
  (int)s; // diagnoses
}
```



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2835
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {
 S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;

Conversion functions?



Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:100
+S s;
+(ConvertTo)s; // expected-warning {{expression result unused}}
+  }

If you specify a message on the `nodiscard` attribute used here, does the 
message get printed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 211094.
erichkeane marked an inline comment as done.
erichkeane added a comment.

Rebased and did all the comments (including the www_status).  @aaron.ballman : 
Wasn't positive what you meant about the conversion functions, but I think I 
got one?  I would also love some bikeshedding on the documentation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -664,7 +664,7 @@
 

 http://wg21.link/p1771r1;>P1771R1 (DR)
-No
+SVN
   
 
   [[maybe_unused]] attribute
Index: clang/test/Preprocessor/has_attribute.cpp
===
--- clang/test/Preprocessor/has_attribute.cpp
+++ clang/test/Preprocessor/has_attribute.cpp
@@ -63,7 +63,7 @@
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
 // WINDOWS: no_unique_address: 0
-// CHECK: nodiscard: 201603L
+// CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // FIXME(201803L) CHECK: unlikely: 0
 
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -80,6 +80,27 @@
   conflicting_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: special reason}}
 }
 
+namespace p1771 {
+  struct [[nodiscard]] ConvertTo{};
+  struct S {
+[[nodiscard]] S();
+S(int);
+[[gnu::warn_unused_result]] S(double);
+operator ConvertTo();
+  };
+
+  struct[[nodiscard]] Y{};
+
+  void usage() {
+S(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+S(1);
+S(2.2);
+Y(); // expected-warning {{expression result unused}}
+S s;
+(ConvertTo)s; // expected-warning {{expression result unused}}
+  }
+}; // namespace p1771
+
 #ifdef EXT
 // expected-warning@5 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@9 {{use of the 'nodiscard' attribute is a C++17 extension}}
@@ -91,4 +112,7 @@
 // expected-warning@71 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@73 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@74 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@84 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@86 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@92 {{use of the 'nodiscard' attribute is a C++17 extension}}
 #endif
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -284,6 +284,13 @@
 return;
   }
 }
+  } else if (const auto *CE = dyn_cast(E)) {
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  if (const auto *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;
+  }
+}
   } else if (ShouldSuppress)
 return;
 
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2831,7 +2831,8 @@
 
 static void handleWarnUnusedResult(Sema , Decl *D, const ParsedAttr ) {
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {
 S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;
 return;
   }
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -2563,13 +2563,31 @@
   case CXXTemporaryObjectExprClass:
   case CXXConstructExprClass: {
 if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
-  if (Type->hasAttr()) {
+  const auto *WarnURAttr = Type->getAttr();
+  if (Type->hasAttr() ||
+  (WarnURAttr && WarnURAttr->IsCXX11NoDiscard())) {
 WarnE = this;
 Loc = getBeginLoc();
 R1 = getSourceRange();
 return true;
   }
 }
+
+const auto *CE = cast(this);
+if (const CXXConstructorDecl *Ctor = 

[PATCH] D64914: Implement P1771

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Please update cxx_status.html to mark P1771R1 as implemented in SVN.




Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:87-88
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@71 {{use of the 'nodiscard' attribute is a C++17 
extension}}
 #endif

aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > Is Core treating this paper as a DR? I don't have a strong opinion here, 
> > > but for the nodiscard with a message version, I made it a C++2a 
> > > extension. I don't have a strong opinion, but I sort of prefer doing 
> > > whatever Core decides.
> > I am unfamiliar with what Core is treating it as, but my understanding is 
> > that EWG encouraged implementers to treat it as such.  
> We expose the attribute in all its glory in all language modes, so these 
> changes already do what we want in effect. The only real question is whether 
> we want to claim it's a C++17 extension or a C++2a extension. If a user turns 
> on extension warnings, we should probably tell them when the feature was 
> added, which is C++2a. It would be a bit weird to claim this is a C++17 when 
> the feature test for it is `__has_attribute(nodiscard) == 201907L` (due to 
> the normative wording changes).
> 
> But if Core moves it as a DR, then C++17 is fine, though I suppose SD-6 would 
> need to make clear what is required for each given attribute feature test 
> value to give us the answer.
We moved this change as a DR, so this feature should be treated as if it were 
always part of the spec.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2833-2835
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {

erichkeane wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > I'm not certain about this -- we do not allow you to put 
> > > > `__attribute__((warn_unused_result))` on a constructor declaration, and 
> > > > neither does GCC. Do you know if they're planning to change their 
> > > > behavior as well? https://godbolt.org/z/kDZu3Q
> > > I'm unsure about GCC's intent, though they will likely be changing the 
> > > [[nodiscard]] behavior due to the vote (and I doubt they will have an "if 
> > > C++20" test either.  
> > > 
> > > Do we have a good way to exclude the alternate spellings?  
> > > isCXX11Attribute only checks syntax, not the actual spelling.  ParsedAttr 
> > > doesn't seem to even contain a way to get which namespace it is 
> > > associated with either.  Is something like that valuable to add?
> > The way we currently do this is `(isCXX11Attribute() || isC2xAttribute()) 
> > && !getScopeName()`, but I am not opposed to going with an additional 
> > member. Do you have a preference either way?
> An additional member of Spelling-Kind (vs Syntax kind) seems very valuable 
> here, though I wonder if it is valuable enough of a change for TableGen.  The 
> isCXX11Attribute && !getScopeName seems a little obtuse from a readers 
> perspective.
Yeah, I would love to solve this through the tablegen interface at some point, 
as that solves other problems we have (like the fact that we don't AST dump 
with the syntax the user wrote). Let's hold off on that for now and use the 
obtuse approach, but with a FIXME for us to come back and touch it again once 
we get tablegen to track more syntactic information along with the semantic 
attribute object.



Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:87-88
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@71 {{use of the 'nodiscard' attribute is a C++17 
extension}}
 #endif

erichkeane wrote:
> aaron.ballman wrote:
> > Is Core treating this paper as a DR? I don't have a strong opinion here, 
> > but for the nodiscard with a message version, I made it a C++2a extension. 
> > I don't have a strong opinion, but I sort of prefer doing whatever Core 
> > decides.
> I am unfamiliar with what Core is treating it as, but my understanding is 
> that EWG encouraged implementers to treat it as such.  
We expose the attribute in all its glory in all language modes, so these 
changes already do what we want in effect. The only real question is whether we 
want to claim it's a C++17 extension or a C++2a extension. If a user turns on 
extension warnings, we should probably tell them when the feature was added, 
which is C++2a. It would be a bit weird to claim this is a C++17 when the 
feature test for it is `__has_attribute(nodiscard) == 201907L` (due to the 
normative wording changes).

But if Core moves it as a DR, then C++17 is fine, though I suppose SD-6 would 
need to make clear what is required for each given attribute feature test value 
to give us the answer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane added a comment.

I'll need to rebase this on another patch soon anyway, so I'll hold off until 
next week to update this particularly since we have some open questions.  The 
additional TableGen work is tempting to do, though I'm not completely sure 
it'll get more than just this use.

I'd also like to see how this paper goes through CWG and see what the general 
attitude is there.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2833-2835
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {

aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > I'm not certain about this -- we do not allow you to put 
> > > `__attribute__((warn_unused_result))` on a constructor declaration, and 
> > > neither does GCC. Do you know if they're planning to change their 
> > > behavior as well? https://godbolt.org/z/kDZu3Q
> > I'm unsure about GCC's intent, though they will likely be changing the 
> > [[nodiscard]] behavior due to the vote (and I doubt they will have an "if 
> > C++20" test either.  
> > 
> > Do we have a good way to exclude the alternate spellings?  isCXX11Attribute 
> > only checks syntax, not the actual spelling.  ParsedAttr doesn't seem to 
> > even contain a way to get which namespace it is associated with either.  Is 
> > something like that valuable to add?
> The way we currently do this is `(isCXX11Attribute() || isC2xAttribute()) && 
> !getScopeName()`, but I am not opposed to going with an additional member. Do 
> you have a preference either way?
An additional member of Spelling-Kind (vs Syntax kind) seems very valuable 
here, though I wonder if it is valuable enough of a change for TableGen.  The 
isCXX11Attribute && !getScopeName seems a little obtuse from a readers 
perspective.



Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:87-88
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@71 {{use of the 'nodiscard' attribute is a C++17 
extension}}
 #endif

aaron.ballman wrote:
> Is Core treating this paper as a DR? I don't have a strong opinion here, but 
> for the nodiscard with a message version, I made it a C++2a extension. I 
> don't have a strong opinion, but I sort of prefer doing whatever Core decides.
I am unfamiliar with what Core is treating it as, but my understanding is that 
EWG encouraged implementers to treat it as such.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2346
+// spellings.
+bool IsCXX11NoDiscard() const {
+  return this->getSemanticSpelling() == CXX11_nodiscard;

I don't think this is strictly required, but perhaps it's not a bad idea.



Comment at: clang/include/clang/Basic/AttrDocs.td:1499
+marked with ``[[nodiscard]]`` or a constructor of a type marked
+``[[nodiscard]]`` will also diagnose.
+

We should probably also talk about type conversions as well.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2833-2835
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {

erichkeane wrote:
> aaron.ballman wrote:
> > I'm not certain about this -- we do not allow you to put 
> > `__attribute__((warn_unused_result))` on a constructor declaration, and 
> > neither does GCC. Do you know if they're planning to change their behavior 
> > as well? https://godbolt.org/z/kDZu3Q
> I'm unsure about GCC's intent, though they will likely be changing the 
> [[nodiscard]] behavior due to the vote (and I doubt they will have an "if 
> C++20" test either.  
> 
> Do we have a good way to exclude the alternate spellings?  isCXX11Attribute 
> only checks syntax, not the actual spelling.  ParsedAttr doesn't seem to even 
> contain a way to get which namespace it is associated with either.  Is 
> something like that valuable to add?
The way we currently do this is `(isCXX11Attribute() || isC2xAttribute()) && 
!getScopeName()`, but I am not opposed to going with an additional member. Do 
you have a preference either way?



Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:69
+[[gnu::warn_unused_result]] S(double);
+  };
+

I think this also needs tests for type conversions, from d1771r1.



Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:87-88
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@71 {{use of the 'nodiscard' attribute is a C++17 
extension}}
 #endif

Is Core treating this paper as a DR? I don't have a strong opinion here, but 
for the nodiscard with a message version, I made it a C++2a extension. I don't 
have a strong opinion, but I sort of prefer doing whatever Core decides.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 210753.
erichkeane marked an inline comment as done.
erichkeane added a comment.

Removes pure/const, limits change to just CXX11 spelling, and changes 
docs/FeatureTestMacro.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
  clang/test/Preprocessor/has_attribute.cpp

Index: clang/test/Preprocessor/has_attribute.cpp
===
--- clang/test/Preprocessor/has_attribute.cpp
+++ clang/test/Preprocessor/has_attribute.cpp
@@ -63,7 +63,7 @@
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
 // WINDOWS: no_unique_address: 0
-// CHECK: nodiscard: 201603L
+// CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // FIXME(201803L) CHECK: unlikely: 0
 
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -61,10 +61,29 @@
 }
 } // namespace PR31526
 
+namespace p1771 {
+  struct S {
+[[nodiscard]] S();
+S(int);
+[[gnu::warn_unused_result]] S(double);
+  };
+
+  struct[[nodiscard]] Y{};
+
+  void usage() {
+S(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+S(1);
+S(2.2);
+Y(); // expected-warning {{expression result unused}}
+  }
+}; // namespace p1771
+
 #ifdef EXT
 // expected-warning@4 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@8 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@11 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@12 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@71 {{use of the 'nodiscard' attribute is a C++17 extension}}
 #endif
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -279,6 +279,13 @@
 return;
   }
 }
+  } else if (const auto *CE = dyn_cast(E)) {
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  if (const auto *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;
+  }
+}
   } else if (ShouldSuppress)
 return;
 
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2831,7 +2831,8 @@
 
 static void handleWarnUnusedResult(Sema , Decl *D, const ParsedAttr ) {
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {
 S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;
 return;
   }
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -2563,13 +2563,31 @@
   case CXXTemporaryObjectExprClass:
   case CXXConstructExprClass: {
 if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
-  if (Type->hasAttr()) {
+  const auto *WarnURAttr = Type->getAttr();
+  if (Type->hasAttr() ||
+  (WarnURAttr && WarnURAttr->IsCXX11NoDiscard())) {
 WarnE = this;
 Loc = getBeginLoc();
 R1 = getSourceRange();
 return true;
   }
 }
+
+const auto *CE = cast(this);
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  const auto *WarnURAttr = Ctor->getAttr();
+  if (WarnURAttr && WarnURAttr->IsCXX11NoDiscard()) {
+WarnE = this;
+Loc = getBeginLoc();
+R1 = getSourceRange();
+
+if (unsigned NumArgs = CE->getNumArgs())
+  R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
+   CE->getArg(NumArgs - 1)->getEndLoc());
+return true;
+  }
+}
+
 return false;
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7426,6 +7426,9 @@
 def warn_unused_call : Warning<
   "ignoring return value of function declared with %0 attribute">,
   InGroup;
+def 

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 5 inline comments as done.
erichkeane added a comment.

In D64914#1591744 , @aaron.ballman 
wrote:

> There seems to be some separable concerns in this patch. I'd rather real with 
> `warn_unused_result`, `const`, and `pure` attributes separately only because 
> those are under the `gnu` attribute namespace and I'd prefer to match GNU's 
> semantics for those attributes (or at least understand better where the 
> inconsistencies are).
>
> This is also missing documentation changes and the updated feature test macro 
> value.


I'll remove the const/pure from this, I was intending to do it for completeness 
and in anticipation of GNU changing their implementation to handle all 3, but I 
dont have reason to believe besides a 'hunch' that they will as well.  As far 
as the warn_unused_result and gnu::warn_unused_result spellings, ParsedAttr 
doesn't seem to have a good way to separate them.  Suggestions?

I'll update the documentation as well with an example of this being used on a 
type/constructor.  The feature test macro (__has_attribute value) wasn't in the 
paper, and it was only altering a note, so I wasn't sure, but seeing as 
nodiscard has its own flag, I'll do so.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2833-2835
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {

aaron.ballman wrote:
> I'm not certain about this -- we do not allow you to put 
> `__attribute__((warn_unused_result))` on a constructor declaration, and 
> neither does GCC. Do you know if they're planning to change their behavior as 
> well? https://godbolt.org/z/kDZu3Q
I'm unsure about GCC's intent, though they will likely be changing the 
[[nodiscard]] behavior due to the vote (and I doubt they will have an "if 
C++20" test either.  

Do we have a good way to exclude the alternate spellings?  isCXX11Attribute 
only checks syntax, not the actual spelling.  ParsedAttr doesn't seem to even 
contain a way to get which namespace it is associated with either.  Is 
something like that valuable to add?



Comment at: clang/lib/Sema/SemaStmt.cpp:288-295
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;
+  }
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;

aaron.ballman wrote:
> GCC does not warn on these cases and does not let you write the attributes on 
> a constructor. Do you know if they're intending to implement this?
I do not know about pure/const, so I've removed them.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

There seems to be some separable concerns in this patch. I'd rather real with 
`warn_unused_result`, `const`, and `pure` attributes separately only because 
those are under the `gnu` attribute namespace and I'd prefer to match GNU's 
semantics for those attributes (or at least understand better where the 
inconsistencies are).

This is also missing documentation changes and the updated feature test macro 
value.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2833-2835
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {

I'm not certain about this -- we do not allow you to put 
`__attribute__((warn_unused_result))` on a constructor declaration, and neither 
does GCC. Do you know if they're planning to change their behavior as well? 
https://godbolt.org/z/kDZu3Q



Comment at: clang/lib/Sema/SemaStmt.cpp:284
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;

`const auto *` here and below?



Comment at: clang/lib/Sema/SemaStmt.cpp:288-295
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;
+  }
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;

GCC does not warn on these cases and does not let you write the attributes on a 
constructor. Do you know if they're intending to implement this?



Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:70
+  };
+  struct [[nodiscard]]Y{};
+  void usage() {

Formatting is a bit messy here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64914/new/

https://reviews.llvm.org/D64914



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

[PATCH] D64914: Implement P1771

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added reviewers: aaron.ballman, chandlerc.
Herald added a project: clang.

EWG met on 7/18/19 and came to the conclusion that our initial intent
was to apply to constructors, so approved the paper and expected
implementers to use implementers discretion to backport this to previous
standards.  ChandlerC was particularly vocal in wanting this.

The intent is to enable the library to start using this on the
constructors of scope_guard/lock_guard.


Repository:
  rC Clang

https://reviews.llvm.org/D64914

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp

Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -61,10 +61,27 @@
 }
 } // namespace PR31526
 
+namespace p1771 {
+  struct S {
+[[nodiscard]] S();
+[[gnu::pure]] S(int);
+[[gnu::const]] S(double);
+  };
+  struct [[nodiscard]]Y{};
+  void usage() {
+S(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+S(1); // expected-warning {{ignoring temporary created by a constructor declared with 'pure' attribute}}
+S(2.2); // expected-warning {{ignoring temporary created by a constructor declared with 'const' attribute}}
+Y(); // expected-warning {{expression result unused}}
+  }
+}; // namespace p1771
+
 #ifdef EXT
 // expected-warning@4 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@8 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@11 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@12 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@70 {{use of the 'nodiscard' attribute is a C++17 extension}}
 #endif
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -279,6 +279,21 @@
 return;
   }
 }
+  } else if (const auto *CE = dyn_cast(E)) {
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;
+  }
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;
+  }
+  if (const Attr *A = Ctor->getAttr()) {
+Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+return;
+  }
+}
   } else if (ShouldSuppress)
 return;
 
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2831,7 +2831,8 @@
 
 static void handleWarnUnusedResult(Sema , Decl *D, const ParsedAttr ) {
   if (D->getFunctionType() &&
-  D->getFunctionType()->getReturnType()->isVoidType()) {
+  D->getFunctionType()->getReturnType()->isVoidType() &&
+  !isa(D)) {
 S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;
 return;
   }
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -2563,13 +2563,30 @@
   case CXXTemporaryObjectExprClass:
   case CXXConstructExprClass: {
 if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
-  if (Type->hasAttr()) {
+  if (Type->hasAttr() ||
+  Type->hasAttr()) {
 WarnE = this;
 Loc = getBeginLoc();
 R1 = getSourceRange();
 return true;
   }
 }
+
+const auto *CE = cast(this);
+if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+  if (Ctor->hasAttr() || Ctor->hasAttr() ||
+  Ctor->hasAttr()) {
+WarnE = this;
+Loc = getBeginLoc();
+R1 = getSourceRange();
+
+if (unsigned NumArgs = CE->getNumArgs())
+  R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
+   CE->getArg(NumArgs - 1)->getEndLoc());
+return true;
+  }
+}
+
 return false;
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7426,6 +7426,9 @@
 def warn_unused_call : Warning<
   "ignoring return value of function declared with %0 attribute">,