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