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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:2906
+                                SkipBodyInfo *SkipBody = nullptr,
+                                bool FnDeleted = false);
   Decl *ActOnStartOfFunctionDef(Scope *S, Decl *D,
----------------
I tend to prefer not doing bool params, and instead some type of 'enum class', as that makes it more clear at the call site.


================
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,
----------------
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).






================
Comment at: clang/lib/Sema/SemaDecl.cpp:14461
   if (!ResultType->isDependentType() && !ResultType->isVoidType() &&
-      !FD->isInvalidDecl() &&
+      !FD->isInvalidDecl() && !FnDeleted &&
       RequireCompleteType(FD->getLocation(), ResultType,
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122981/new/

https://reviews.llvm.org/D122981