[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 <https://reviews.llvm.org/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<CXXConstructorDecl>(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<PureAttr>()) {
+        Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+        return;
+      }
+      if (const Attr *A = Ctor->getAttr<ConstAttr>()) {
+        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