[PATCH] D122981: [Clang] Diagnose incomplete return/param types only when function is not deleted

[PoYao Chang via Phabricator via cfe-commits]

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