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

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao marked 2 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:
> rZhBoYao wrote:
> > 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`.
> My point is: do that parsing in this function BEFORE the call to ActOnStartOfFunctionDef?
> 
> Alternatively, we could add a function to Sema to 'ActOnFunctionDefinition(DefType)' and move this diagnostic THERE instead of ActOnStartofFunctionDef, and call it AFTER we have handled the '= delete/=default/etc'.
> do that parsing in this function BEFORE the call to ActOnStartOfFunctionDef?
But we need Decl *Res returned by ActOnStartOfFunctionDef.

I did try to factor out the diagnostic right after `ActOnFunctionDefinition` and 27 tests were failing.
Furthermore, we are not the only caller of `ActOnFunctionDefinition`.


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

https://reviews.llvm.org/D122981