[PATCH] D79160: [AST] Preserve the type in RecoveryExprs for broken function calls.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 30 05:38:54 PDT 2020
sammccall marked an inline comment as done.
sammccall added inline comments.
================
Comment at: clang/include/clang/AST/Expr.h:6042
///
/// For now, RecoveryExpr is type-, value- and instantiation-dependent to take
/// advantage of existing machinery to deal with dependent code in C++, e.g.
----------------
Comment is stale.
================
Comment at: clang/include/clang/AST/Expr.h:6042
///
/// For now, RecoveryExpr is type-, value- and instantiation-dependent to take
/// advantage of existing machinery to deal with dependent code in C++, e.g.
----------------
sammccall wrote:
> Comment is stale.
We should mention something about optional type preservation.
================
Comment at: clang/lib/AST/ComputeDependence.cpp:491
+ auto D =
+ toExprDependence(E->getType()->getDependence()) | ExprDependence::Error;
for (auto *S : E->subExpressions())
----------------
Dropping type-dependence seems like the point here and is risky, doesn't dropping value- and instantiation-dependence introduce unneccesary extra risk?
Probably correct and maybe useful, but splitting it into a separate change and maybe delaying it might make it easier to get the principal change to stick.
================
Comment at: clang/lib/AST/Expr.cpp:4656
#ifndef NDEBUG
+ assert(!T.isNull());
for (auto *E : SubExprs)
----------------
Why is this in ifndef?
Actually, why is the loop in ifndef? surely the compiler can inline and remove an empty loop over an arrayref?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:16427
QualType EltTy = Context.getBaseElementType(T);
- if (!EltTy->isDependentType()) {
+ if (!EltTy->isDependentType() && !EltTy->containsErrors()) {
if (RequireCompleteSizedType(Loc, EltTy,
----------------
why is this needed/does it help?
I would have thought in the cases where we now don't consider the type dependent, the *type* wouldn't contain errors (rather the expression owning the type would).
Is this just for decltype?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:12904
- // Overload resolution failed.
- return ExprError();
+ // Overload resolution failed, try to recovery.
+ SmallVector<Expr *, 8> SubExprs = {Fn};
----------------
nit: try to recover
(or just "Overload resolution failed", what the code is doing is obvious enough here)
================
Comment at: clang/test/CodeCompletion/member-access.cpp:276
+struct S { int member; };
+S overloaded(int);
----------------
I think there's an interesting test of the "viable" case where you have a const/non-const overload set with different return types:
```
class Collection {
const char *find(int) const;
char* find(int) const;
};
void test1(const Collection &C, Collection &NC) {
C.find(); // missing arg, type const char*
NC.find(); // missing arg, is type NC or is it unresolved?
}
```
(Not sure if it's best written as a codecompletion test, I guess AST dump is better)
================
Comment at: clang/test/Index/getcursor-recovery.cpp:6
+void testTypedRecoveryExpr1() {
+ // Inner bar() is a RecoveryExpr, outer foo() is an overloaded call.
+ foo(x, bar(x));
----------------
nit: unresolved overloaded call?
================
Comment at: clang/test/Index/getcursor-recovery.cpp:19
+void testTypedRecoveryExpr2() {
+ // Inner foo() is a RecoveryExpr (with int type), outer foo() is a "foo(int, int)" call.
+ foo(x, foo(x));
----------------
nit: a valid foo(int, int) call?
(just to draw the contrast)
================
Comment at: clang/test/SemaCXX/enable_if.cpp:417
-constexpr int B = 10 + // the carat for the error should be pointing to the problematic call (on the next line), not here.
- callTemplated<0>(); // expected-error{{initialized by a constant expression}} expected-error at -3{{no matching function for call to 'templated'}} expected-note{{in instantiation of function template}} expected-note at -10{{candidate disabled}}
+constexpr int B = 10 + // expected-error {{initialized by a constant expression}}
+ callTemplated<0>(); // expected-error at -3{{no matching function for call to 'templated'}} expected-note{{in instantiation of function template}} expected-note at -10{{candidate disabled}} expected-note {{in call to 'callTemplated()'}} expected-note at -3 {{subexpression not valid in a constant expression}}
----------------
hokein wrote:
> this diagnostic is changed slightly (without `-frecovery-ast-type`). I think this is acceptable.
>
> before this patch:
>
> ```
> /tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
> return templated<N>(); // expected-error {{no matching function for call to 'templated'}}
> ^~~~~~~~~~~~
> /tmp/t5.cpp:13:5: note: in instantiation of function template specialization 'enable_if_diags::callTemplated<0>' requested here
> callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note at -6 {{subexpression not valid in a constant expression}}
> ^
> /tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
> template <int N> constexpr int templated() __attribute__((enable_if(N, ""))) {
> ^ ~
> /tmp/t5.cpp:13:5: error: constexpr variable 'B' must be initialized by a constant expression
> callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note at -6 {{subexpression not valid in a constant expression}}
> ^~~~~~~~~~~~~~~~~~
> 2 errors generated.
> ```
>
> vs after this patch:
>
> ```
> /tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
> return templated<N>(); // expected-error {{no matching function for call to 'templated'}}
> ^~~~~~~~~~~~
> /tmp/t5.cpp:13:5: note: in instantiation of function template specialization 'enable_if_diags::callTemplated<0>' requested here
> callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note at -6 {{subexpression not valid in a constant expression}}
> ^
> /tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
> template <int N> constexpr int templated() __attribute__((enable_if(N, ""))) {
> ^ ~
> /tmp/t5.cpp:12:15: error: constexpr variable 'B' must be initialized by a constant expression
> constexpr int B = 10 + // expected-error {{constexpr variable 'B' must be initialized by a constant expression}}
> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /tmp/t5.cpp:7:10: note: subexpression not valid in a constant expression
> return templated<N>(); // expected-error {{no matching function for call to 'templated'}}
> ^
> /tmp/t5.cpp:13:5: note: in call to 'callTemplated()'
> callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}} expected-note at -6 {{subexpression not valid in a constant expression}}
> ^
> 2 errors generated.
> ```
>
>
Yeah, this is noisier but seems OK.
Can you take a look at the blame for the split-line test and see if there was any special code to support it that can be cleaned up?
================
Comment at: clang/test/SemaCXX/recovery-expr-type.cpp:12
+ // no crash.
+ int s = sizeof(make_indestructible()); // expected-error {{deleted}}
+}
----------------
what if we make this constexpr and try to static-assert on it, use it as the size of an array etc?
================
Comment at: clang/test/SemaTemplate/instantiate-function-params.cpp:1
// RUN: %clang_cc1 -triple i686-unknown-unknown -fsyntax-only -verify %s
----------------
hokein wrote:
> this test is very tricky, and hard for human to understand. It was added to test clang not crashing.
>
> Different compilers (gcc, msvc) emit different diagnostics, with this patch, clang emits 3 more errs `no type named 'type'...`, it seems reasonable, and gcc also emits them.
Ugh, I remember this test. It's partially reduced from boost IIRC.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79160/new/
https://reviews.llvm.org/D79160
More information about the cfe-commits
mailing list