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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
