[PATCH] D32207: Corrrect warn_unused_result attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D32207#730681, @erichkeane wrote:

> 1 more thing:  I excepted post-increment/decrement from this warning because I figured it would give a TON of warnings from people using post-inc/dec in for loops.
>
> However, after further discussion with Craig (who brought this up on the mailing list), I severely wonder if that is the 'right' thing to do.  Someone who is going to mark their struct in this way would likely ALSO mean it in this case, right?
>
> Aaron: Should I simply remove the special casing all together, so that it warns on post inc/dec?  That would definitely solve all your concerns in the review so far.


I think that would make sense, yes.



================
Comment at: lib/AST/Decl.cpp:3010
+        (OpCode == OO_PlusPlus || OpCode == OO_MinusMinus) &&
+        (this->getNumParams() + (isa<CXXMethodDecl>(this) ? 1 : 0)) == 2;
+    if (Ret && !IsPostfix) {
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > CXXMethodDecl represents a static or instance method, so I'm not certain this logic is quite correct.
> I actually stole this logic from lib/Sema/SemaDeclCXX.cpp lines 12708 and 12754.  I believe the logic here is that it operators aren't allowed to be static?  Since it is illegal to do so,  I would expect that I'd simply need to do an assert to prevent it?
> 
> I'm open for ideas if you have one here, I'm not sure what the 'right' thing to do is.
I was wondering more how a friend declaration of the operator might show up. e.g.,
```
struct S {
  friend S operator++(S Operand, int);
};
```


================
Comment at: test/SemaCXX/warn-unused-result.cpp:166
+// are special-cased to not warn for return-type due to ubiquitousness.
+struct[[clang::warn_unused_result]] S {
+  S DoThing() { return {}; };
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > Can you also add a test with [[nodiscard]] and verify that the uses of increment/decrement *are* diagnosed?
> I can add that, it will obviously cause a slight change in logic since I didn't handle that separately.  Are we surewe WANT [[nodiscard]] to warn in this case?
I think so, yes. The C++ standard doesn't provide normative requirements here, but its encouragement to implementers doesn't mention this kind of behavior. [dcl.attr.nodiscard]p2:
```
A nodiscard call is a function call expression that calls a function previously declared nodiscard, or
whose return type is a possibly cv-qualified class or enumeration type marked nodiscard. Appearance of a nodiscard call as a potentially-evaluated discarded-value expression (Clause 8) is discouraged unless explicitly cast to void. Implementations are encouraged to issue a warning in such cases. This is typically because discarding the return value of a nodiscard call has surprising consequences.
```
Since this is returning a class type that is marked as nodiscard, I would expect it to be diagnosed with the standard attribute spelling.


https://reviews.llvm.org/D32207