[PATCH] D85716: [AST][RecoveryExpr] Fix a bogus unused diagnostic when the type is preserved.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/test/SemaCXX/recovery-expr-type.cpp:88
+void func() {
+  // verify that no -Wunused-variable diagnostic on the inner Derived expression.
+  (Derived(Derived())); // expected-error {{call to implicitly-deleted default constructor}}
----------------
sammccall wrote:
> sammccall wrote:
> > (nit: I think this is -Wunused but not -Wunused-variable)
> what makes you think this is firing on the *inner* expression? The diagnostic location is that of the outer expression. I think the existing logic is pretty close to correct (and the diagnostic is actually correct here, albeit through luck), and if we want to change it, the right fix belongs somewhere else.
> 
> The warning doesn't fire for just `Derived();` - isUnusedResultAWarning returns false for RecoveryExpr. Nor does it fire for `(Derived());` - isUnusedResultAWarning looks inside the paren expr and gets the same result for the contents.
> 
> For the case in this test, we hit the CXXFunctionalCastExpr/CStyleExprCast case of isUnusedResultAWarning. It says:
> 
> > If this is a cast to a constructor conversion, check the operand.
> > Otherwise, the result of the cast is unused.
> 
> But this is a bit misleading: given `Foo(2+2)` the "operand" isn't 2+2, but rather a CXXConstructExpr.  And isUnusedResultAWarning returns true for this case (usually).
> 
> Back to the case at hand: we diverge because the cast type is Dependent instead of ConstructorConversion.
> This seems like it could be a bug - in this configuration the operand is not type-dependent so why can't it resolve the overload?
> But in any case, I don't think isUnusedResultAWarning should be assuming a dependent cast is safe to warn on - rather either return false or delegate to the inner expr (which in this case is a RecoveryExpr).
> 
oh, my bad. I misunderstood this part of code -- I missed the critical function `isUnusedResultAWarning`, I saw it when reading this part of code, but I skipped it because the function name made me think "this is a trivial function that just detects whether the -Wunused flag is on".

I think you analysis is right here. Thanks!

> what makes you think this is firing on the *inner* expression? The diagnostic location is that of the outer expression.

hmm, I was confused by the ~annotation of the diagnostic.

```
(Derived(Derived()));
  ^      ~~~~~~~
``` 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85716/new/

https://reviews.llvm.org/D85716