sammccall added a comment.
Cool, this makes sense to me. I hadn't realized this doesn't go through
typoexpr, that's a bit unfortunate we can't address this just in one place.
This is a change of behavior with recoveryAST off, but this change basically
only affects C++ so that's not a widely-used configuration.
I think the resulting AST is better as the errors there are visible.
Can we have a test specifically of the shape of the AST produced?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12729
///
/// Returns true if new candidates were found.
static ExprResult
----------------
while here, drop the stale "returns true" bit? The return type covers the
contract well enough IMO
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12786
// Build an implicit member call if appropriate. Just drop the
// casts and such from the call, we don't really care.
----------------
nit: now member call -> member access as we never really go on to build a
member call
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12801
+ // Offering "follow-up" diagnostics on these recovery call expressions seems
+ // like a bad risk/reward tradeoff, e.g. following diagnostics on a
----------------
This seems a bit abrupt (because it's not obvious what the previous code is
doing I guess).
Maybe
"We now have recovered a callee. However, building a real call may lead to
incorrect
secondary diagnostics if our recovery wasn't correct.
We keep the recovery diagnostics and AST but suppress following diagnostics by
using RecoveryExpr"
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12806
+ // using RecoveryExpr.
+ NewFn = SemaRef.CreateRecoveryExpr(NewFn.get()->getBeginLoc(),
+ NewFn.get()->getEndLoc(), {NewFn.get()});
----------------
I think giving the variable a new name here would be clearer
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12811
+
// This shouldn't cause an infinite loop because we're giving it
// an expression with viable lookup results, which should never
----------------
comment is now out of date.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12814
// end up here.
return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
MultiExprArg(Args.data(), Args.size()),
----------------
This results in (IIUC):
```
CallExpr
>RecoveryExpr (indirection we inserted)
>>DeclRefExpr (callee)
>Arg1
```
Whereas when overload resolution fails, we create:
```
RecoveryExpr (call)
> OverloadExpr (callee)
> Arg1
```
I can see advantages for either, but is there a reason not to be consistent?
(Which I guess means emitting the first one here)
================
Comment at: clang/test/Modules/submodules-merge-defs.cpp:21
extern class A pre_a2;
-int pre_use_a = use_a(pre_a2); // expected-error 2{{'A' must be defined}}
expected-error {{'use_a' must be declared}}
+int pre_use_a = use_a(pre_a2); // expected-error {{'use_a' must be declared}}
// expected-note@defs.h:2 +{{here}}
----------------
hokein wrote:
> this (and the case below) are slight regressions, but I think the following
> diagnostics are not a big deal.
I agree, the secondary diagnostics aren't wrong but I don't think they're
necessary either.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89946/new/
https://reviews.llvm.org/D89946
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
