[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<CXXConstructorDecl>(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 at 28 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning at 66 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning at 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