[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<CXXConstructorDecl>(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 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
----------------
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