[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/lib/Parse/Parser.cpp:1306
+  bool Delete =
+      Tok.is(tok::equal) && NextToken().is(tok::kw_delete) ? true : false;
   Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,
----------------
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'.


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

https://reviews.llvm.org/D122981