hokein marked 2 inline comments as done. hokein added inline comments.
================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1688 CheckConstexprKind Kind) { + assert(!NewFD->isInvalidDecl()); const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD); ---------------- rsmith wrote: > Instead of asserting this, please just return false on an invalid > declaration. There's nothing fundamentally wrong with calling this function > on an invalid declaration, but as usual, if we encounter something invalid, > we should suppress follow-on diagnostics. oh, thanks. This function is only called in two places, and both of them check the NewFD->isValid before calling it, so my understanding is that this function is required a valid NewFD. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005 + if (Member->getInit() && Member->getInit()->containsErrors()) + Constructor->setInvalidDecl(); if (Member->isBaseInitializer()) ---------------- 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. 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