sammccall added a comment.
This makes me nervous, marking the constructor as invalid seems much safer. Can
you show tests it regresses?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:14359
+ !ErrorsInCtorInitializer &&
!CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose))
FD->setInvalidDecl();
----------------
hokein wrote:
> The crash is in `CheckConstexprFunctionDefinition`, I tried different ways to
> fixing it:
>
> 1) mark `FD` invalid when there are any errors in CtorInitailizer -- clang
> deliberately treats CtorDecl as valid even there are some errors in the
> initializer to prevent spurious diagnostics (see the cycle delegation in the
> test as an example), so marking them invalid may affect the quality of
> diagnostics;
>
> 2) Fixing it inside `CheckConstexprFunctionDefinition` or
> `isPotentialConstantExpr`, but it doesn't seem to be a right layer, these
> functions are expected to be called on a validDecl (or at least after a few
> sanity checks), and emit diagnostics.
> clang deliberately treats CtorDecl as valid even there are some errors in the
> initializer
Do you mean currently? (when such errors result in destroying the whole init
expr)
If so, it would be nice to preserve this indeed.
I'm worried that we're going to decide that a constexpr constructor is valid
and then actually try to evaluate it at compile time.
What happens if we try to evaluate `constexpr int Z = X().Y` in your testcase?
> Fixing it inside CheckConstexprFunctionDefinition
I have a couple of concerns with where it's currently implemented:
- aren't there lots of other paths we're going to call isPotentialConstantExpr
and crash? CheckConstexprFunctionDefinition is large and complicated,
init-lists are only a small part.
- if we treat anything with errors in the ctor as valid (am I reading that
right?!) then won't that mask *other* problems where the non-error parts of the
definition should cause it to be treated as invalid? e.g. `Foo() :
m1(broken_but_maybe_constexpr()), m2(exists_and_not_constexpr()) {}`
Wherever this goes, if we're going to do something subtle like marking
diagnostics as valid based on errors in them, it deserves a comment laying out
the cases.
================
Comment at: clang/test/SemaCXX/invalid-constructor-init.cpp:17
+ CycleDelegate(int) : Y(foo()) {} // expected-error {{use of undeclared
identifier 'foo'}}
+ // no "delegation cycle" diagnostic emitted!
+ CycleDelegate(float) : CycleDelegate(1) {}
----------------
what's the behavior with -fno-recovery-ast?
(It'd be nice to improve it, failing to improve it is OK. regressing is
probably bad...)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77041/new/
https://reviews.llvm.org/D77041
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
