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

Reply via email to