sammccall added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005
+ if (Member->getInit() && Member->getInit()->containsErrors())
+ Constructor->setInvalidDecl();
if (Member->isBaseInitializer())
----------------
hokein wrote:
> rsmith wrote:
> > rsmith wrote:
> > > This is inappropriate. The "invalid" bit on a declaration indicates
> > > whether the declaration is invalid, not whether the definition is invalid.
> > >
> > > What we should do is add a "contains errors" bit to `Stmt`, and mark the
> > > function body as containing errors if it has an initializer that contains
> > > an error.
> > As an example of why this distinction matters: the "contains errors" bit
> > indicates whether external users of the declaration should ignore it /
> > suppress errors on it, or whether they can still treat it as a regular
> > declaration. It's not ideal for an error in the body of a function to
> > affect the diagnostic behavior of a caller to that function, since the body
> > is (typically) irrelevant to the validity of that caller.
> Thanks for the suggestions and clarifications! The "invalid" bit of decl is
> subtle, I didn't infer it from the code before, thanks for the explanation.
>
> Setting the decl invalid seemed much safer to us, and would avoid running a
> lot of unexpected code paths in clang which might violate the assumptions.
>
> > What we should do is add a "contains errors" bit to Stmt, and mark the
> > function body as containing errors if it has an initializer that contains
> > an error.
>
> This sounds promising, but there are no free bit in Stmt at the moment :( (to
> add the ErrorBit to expr, we have sacrificed the `IsOMPStructuredBlock` bit,
> it would be easier after the ongoing FPOptions stuff finished, which will
> free certain bits).
>
> since this crash is blocking recovery-expr stuff, it is prioritized for us to
> fix it. Maybe a compromising plan for now is to fix it in
> `CheckConstexprFunctionDefinition` (just moving the inspecting `InitExpr`
> code there) -- the crash is from `isPotentialConstantExpr` which is only
> called in `CheckConstexprFunctionDefinition`, should cover all cases.
I don't think the availability or otherwise of a bit in stmt is the critical
factor here. Adding a bit to stmt will be a huge amount of work because (unlike
expr/type) stmts don't have dependence, so there's no existing dependence
propagation/handling to reuse.
When a ctor has an init expr with an error, I think we ultimately need to
choose between:
1. ctor is constant-evaluable. (This seems obviously wrong)
2. ctor is not-constant-evaluable. (This creates spurious "must be a constant
expression" diags)
3. ctor is invalid and therefore we never ask. (This creates other spurious
diags due to not finding the ctor)
4. ctor-with-errors is handled specially (we find it but don't check it for
constant-evaluability).
Adding a stmt bit is 4, and is certainly the best long-term direction. 2 seems
like a reasonable short-term solution though. Can we modify the is-constexpr
check to return false if errors are present, before asserting there's no
dependent code?
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
