[PATCH] D82657: [AST][RecoveryAST] Preserve the type by default for recovery expression.

Wed Aug 12 00:54:47 PDT 2020

hokein added a comment.

> As you said, we can't land this before the branch cut, and we shouldn't land this until we've run internal experiments to show it's not horribly crashy.

The internal experiment result is good, I think we're close to land it. After a new rebase, we need to adjust more diagnostics, but all of them look like good secondary-diagnostic improvements.

================
Comment at: clang/test/CXX/temp/temp.decls/temp.variadic/fixed-expansion.cpp:129
+  S<int, int, double> &s1 = f({}, 0, 0.0); // expected-error {{no matching function}} \
+                                              expected-error {{non-const lvalue reference to type 'S<int, int, double>' cannot bind to a value of unrelated type 'int'}}
 }
----------------
sammccall wrote:
> hokein wrote:
> > the secondary diagnostic is technically correct, but I don't quite like it, it is confusing, ok to leave it as-is? or fix that before landing this patch?
> I don't think it's technically correct (the `int` fallback is an implementation artifact, albeit one that leaks quite often).
> This example looks pretty obscure though, it'd be nice to fix it at some point but I don't think it's severe enough to block on the fix.
> 
> Fix ideas:
>  - the instantiation of `f` should be invalid, right? Maybe we avoid considering return types for invalid decls when computing recovery type (or always give up in this case).
>  - we could eventually try to replace the use of `int` for these cases
>  - we could special-case `int`, and don't allow RecoveryExprs to have type `int`, reflecting the fact that it's used for these cases
Fixed in D85714.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82657