rZhBoYao marked 3 inline comments as done. rZhBoYao added inline comments.
================ Comment at: clang/lib/Parse/Parser.cpp:1306 + bool Delete = + Tok.is(tok::equal) && NextToken().is(tok::kw_delete) ? true : false; Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D, ---------------- erichkeane wrote: > I'm not sure about doing this 'look ahead' here, this feels dangerous to me. > First, does this work with comments? Second, it seems we wouldn't normally > look at 'deleted' if SkipBody.ShouldSkip (see below with the early exit)? > > Next I'm not a fan of double-parsing these tokens with this lookahead. I > wonder, if we should move hte logic from ~1334 and 1338 up here and calculate > the 'deleted'/'defaulted' 'earlier', before we 'actOnStartOfFunctionDef`. > > This would result in us being able to instead change the signature of > ActOnStartOfFunctionDef to take some enum as to whether it is > deleted/defaulted, AND create the function decl as deleted/defaulted 'in > place' (or, at least, call SetDeclDeleted or SetDeclDefaulted immediately). > > > > > I'm not sure about doing this 'look ahead' here, this feels dangerous to me. > First, does this work with comments? Yes, it returns a normal token after phase 5, so comments are long gone. > Second, it seems we wouldn't normally look at 'deleted' if > SkipBody.ShouldSkip (see below with the early exit)? SkipBody.ShouldSkip is an output parameter of `ActOnStartOfFunctionDef`. We need to either look ahead or consume "delete" before entering `ActOnStartOfFunctionDef` anyway. > Next I'm not a fan of double-parsing these tokens with this lookahead. It does look weird. Consume them I will. Updated diff coming. > AND create the function decl as deleted/defaulted 'in place' (or, at least, > call SetDeclDeleted or SetDeclDefaulted immediately). SetDecl{Deleted | Defaulted} needs KWLoc tho. I haven't thought of a way of doing that "in place" inside `ActOnStartOfFunctionDef`. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:14461 if (!ResultType->isDependentType() && !ResultType->isVoidType() && - !FD->isInvalidDecl() && + !FD->isInvalidDecl() && !FnDeleted && RequireCompleteType(FD->getLocation(), ResultType, ---------------- erichkeane wrote: > ChuanqiXu wrote: > > rZhBoYao wrote: > > > ChuanqiXu wrote: > > > > I think we could remove the use of `FnDeleted` by the use of > > > > `FD->isDeleted()`. Also we should constrain the behavior only if std >= > > > > 11. > > > I tried `FD->isDeleted()` at first only to find out it always returns > > > `false`. Looks like it's not handled until [[ > > > https://github.com/llvm/llvm-project/blob/634bf829a8d289371d5b5a50b787596124228898/clang/lib/Parse/Parser.cpp#L1342 > > > | Parser.cpp:1342 ]] > > > > > > Also, looks like deleted functions are implemented as an extension. A > > > warning is issued at [[ > > > https://github.com/llvm/llvm-project/blob/634bf829a8d289371d5b5a50b787596124228898/clang/lib/Parse/Parser.cpp#L1338-L1341 > > > | Parser.cpp:1338 ]] if language mode < C++11 > > I see. My suggestion might be to defer the diagnose message after we set > > `FD->setDeletedAsWritten()`. I understand it might be harder. But the > > current implementation looks a little bit not good... (redundant variables > > and passing information by argument) > I disagree on doing this for C++11 mode. As we allow them as an extension, > we want this to work in the extension mode as well. Thanks for the endorsement. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122981/new/ https://reviews.llvm.org/D122981 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits