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

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao updated this revision to Diff 421328.
rZhBoYao marked 5 inline comments as done.
rZhBoYao added a comment.

Removed member function test cases and addressed comments,
which includes:

1. Sema::SetFunctionBodyKind
2. Change enum names
3. Be clear about delete being C++ specific.


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

https://reviews.llvm.org/D122981

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp

Index: clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
===
--- /dev/null
+++ clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct Incomplete; // expected-note 2{{forward declaration of 'Incomplete'}}
+Incomplete f(Incomplete) = delete; // well-formed
+Incomplete g(Incomplete) {}// expected-error{{incomplete result type 'Incomplete' in function definition}}\
+// expected-error{{variable has incomplete type 'Incomplete'}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -17379,6 +17379,21 @@
   }
 }
 
+void Sema::SetFunctionBodyKind(Decl *D, SourceLocation Loc,
+   FnBodyKind BodyKind) {
+  switch (BodyKind) {
+  case FnBodyKind::Delete:
+SetDeclDeleted(D, Loc);
+break;
+  case FnBodyKind::Default:
+SetDeclDefaulted(D, Loc);
+break;
+  case FnBodyKind::Other:
+llvm_unreachable(
+"Parsed function body should be '= delete;' or '= default;'");
+  }
+}
+
 bool Sema::CheckOverridingFunctionAttributes(const CXXMethodDecl *New,
  const CXXMethodDecl *Old) {
   const auto *NewFT = New->getType()->castAs();
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14185,7 +14185,7 @@
 Decl *
 Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Declarator ,
   MultiTemplateParamsArg TemplateParameterLists,
-  SkipBodyInfo *SkipBody) {
+  SkipBodyInfo *SkipBody, FnBodyKind BodyKind) {
   assert(getCurFunctionDecl() == nullptr && "Function parsing confused");
   assert(D.isFunctionDeclarator() && "Not a function declarator!");
   Scope *ParentScope = FnBodyScope->getParent();
@@ -14204,7 +14204,7 @@
 
   D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
-  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody);
+  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody, BodyKind);
 
   if (!Bases.empty())
 ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(Dcl, Bases);
@@ -14380,7 +14380,8 @@
 }
 
 Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
-SkipBodyInfo *SkipBody) {
+SkipBodyInfo *SkipBody,
+FnBodyKind BodyKind) {
   if (!D) {
 // Parsing the function declaration failed in some way. Push on a fake scope
 // anyway so we can try to parse the function body.
@@ -14469,11 +14470,11 @@
 }
   }
 
-  // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // The return type of a function definition must be complete (C99 6.9.1p3),
+  // unless the function is deleted (C++ specifc, C++ [dcl.fct.def.general]p2)
   QualType ResultType = FD->getReturnType();
   if (!ResultType->isDependentType() && !ResultType->isVoidType() &&
-  !FD->isInvalidDecl() &&
+  !FD->isInvalidDecl() && BodyKind != FnBodyKind::Delete &&
   RequireCompleteType(FD->getLocation(), ResultType,
   diag::err_func_def_incomplete_result))
 FD->setInvalidDecl();
@@ -14482,8 +14483,9 @@
 PushDeclContext(FnBodyScope, FD);
 
   // Check the validity of our function parameters
-  CheckParmsForFunctionDef(FD->parameters(),
-   /*CheckParameterNames=*/true);
+  if (BodyKind != FnBodyKind::Delete)
+CheckParmsForFunctionDef(FD->parameters(),
+ /*CheckParameterNames=*/true);
 
   // Add non-parameter declarations already in the function to the current
   // scope.
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1299,6 +1299,41 @@
   ParseScope BodyScope(this, Scope::FnScope | Scope::DeclScope |
  Scope::CompoundStmtScope);
 
+  // Parse function body 

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This definitely looks like it is 'nicer' than before,  a few smaller/nit-ish 
comments.

Additionally, Phab made a REAL mess of the diff, if you could give a quick 
summary of what changes were actually made (vs things that were moved slightly 
and causing massive red/green diffs), it would be helpful.




Comment at: clang/include/clang/Sema/Sema.h:2906
+
+/// Not yet parsed
+/// Could be one of:





Comment at: clang/include/clang/Sema/Sema.h:2910
+///   function-try-block
+NotYetParsed
+  };

Might suggest 'Other'?  

Also, the EqDelete and EqDefault could probably just be Delete/Default and be 
equally as clear.



Comment at: clang/lib/Parse/Parser.cpp:1363
+if (BodyKind == Sema::FnBodyKind::EqDelete)
+  Actions.SetDeclDeleted(Res, KWLoc);
+else if (BodyKind == Sema::FnBodyKind::EqDefault)

Since the FnBodyKind is in Sema, I would suggest just adding a new function to 
replace all of this, "SetFunctionBodyKind(Res, BodyKind)", and make Sema do the 
'switch' here.



Comment at: clang/lib/Sema/SemaDecl.cpp:14474
   // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // unless the function is deleted
+  // (C99 6.9.1p3, C++ [dcl.fct.def.general]p2).

This is C++ specific, so could we be specific about that here?


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

https://reviews.llvm.org/D122981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao marked an inline comment 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,

rZhBoYao wrote:
> 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`.
I meant to say `ActOnStartOfFunctionDef`.


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

https://reviews.llvm.org/D122981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao updated this revision to Diff 421305.
rZhBoYao added a comment.

Handling of eagerly parsed deleted or defaulted function must happen AFTER 
`D.complete(Res);`.

A nice looking diff: 
https://github.com/poyaoc97/llvm-project/commit/dc37a262582f6a2d8143c6f1f586dc657b4b5311


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

https://reviews.llvm.org/D122981

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp

Index: clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
===
--- /dev/null
+++ clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct Incomplete; // expected-note 4{{forward declaration of 'Incomplete'}}
+Incomplete f(Incomplete) = delete; // well-formed
+Incomplete g(Incomplete) {}// expected-error{{incomplete result type 'Incomplete' in function definition}}\
+// expected-error{{variable has incomplete type 'Incomplete'}}
+
+struct C {
+  Incomplete f(Incomplete) = delete; // well-formed
+  Incomplete g(Incomplete) {}// expected-error{{incomplete result type 'Incomplete' in function definition}}\
+  // expected-error{{variable has incomplete type 'Incomplete'}}
+};
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14185,7 +14185,7 @@
 Decl *
 Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Declarator ,
   MultiTemplateParamsArg TemplateParameterLists,
-  SkipBodyInfo *SkipBody) {
+  SkipBodyInfo *SkipBody, FnBodyKind BodyKind) {
   assert(getCurFunctionDecl() == nullptr && "Function parsing confused");
   assert(D.isFunctionDeclarator() && "Not a function declarator!");
   Scope *ParentScope = FnBodyScope->getParent();
@@ -14204,7 +14204,7 @@
 
   D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
-  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody);
+  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody, BodyKind);
 
   if (!Bases.empty())
 ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(Dcl, Bases);
@@ -14380,7 +14380,8 @@
 }
 
 Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
-SkipBodyInfo *SkipBody) {
+SkipBodyInfo *SkipBody,
+FnBodyKind BodyKind) {
   if (!D) {
 // Parsing the function declaration failed in some way. Push on a fake scope
 // anyway so we can try to parse the function body.
@@ -14470,10 +14471,11 @@
   }
 
   // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // unless the function is deleted
+  // (C99 6.9.1p3, C++ [dcl.fct.def.general]p2).
   QualType ResultType = FD->getReturnType();
   if (!ResultType->isDependentType() && !ResultType->isVoidType() &&
-  !FD->isInvalidDecl() &&
+  !FD->isInvalidDecl() && BodyKind != FnBodyKind::EqDelete &&
   RequireCompleteType(FD->getLocation(), ResultType,
   diag::err_func_def_incomplete_result))
 FD->setInvalidDecl();
@@ -14482,8 +14484,9 @@
 PushDeclContext(FnBodyScope, FD);
 
   // Check the validity of our function parameters
-  CheckParmsForFunctionDef(FD->parameters(),
-   /*CheckParameterNames=*/true);
+  if (BodyKind != FnBodyKind::EqDelete)
+CheckParmsForFunctionDef(FD->parameters(),
+ /*CheckParameterNames=*/true);
 
   // Add non-parameter declarations already in the function to the current
   // scope.
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1299,6 +1299,41 @@
   ParseScope BodyScope(this, Scope::FnScope | Scope::DeclScope |
  Scope::CompoundStmtScope);
 
+  // Parse function body eagerly if it is either '= delete;' or '= default;' as
+  // ActOnStartOfFunctionDef needs to know whether the function is deleted.
+  Sema::FnBodyKind BodyKind = Sema::FnBodyKind::NotYetParsed;
+  SourceLocation KWLoc;
+  if (TryConsumeToken(tok::equal)) {
+assert(getLangOpts().CPlusPlus && "Only C++ function definitions have '='");
+
+if (TryConsumeToken(tok::kw_delete, KWLoc)) {
+  Diag(KWLoc, getLangOpts().CPlusPlus11
+  ? diag::warn_cxx98_compat_defaulted_deleted_function
+  : diag::ext_defaulted_deleted_function)
+  << 1 /* deleted */;
+  BodyKind = Sema::FnBodyKind::EqDelete;
+} else if 

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D122981#3432233 , @rZhBoYao wrote:

> I think an extra parameter is inevitable without complicating things too much.

My thought is that it might not be worth to fix the problem for the workaround. 
I feel like the problem is less countered. Let's try to fix it in a cleaner way.


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

https://reviews.llvm.org/D122981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao updated this revision to Diff 420764.
rZhBoYao marked 2 inline comments as done.
rZhBoYao added a reviewer: erichkeane.
rZhBoYao added a comment.

I think an extra parameter is inevitable without complicating things too much.


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

https://reviews.llvm.org/D122981

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp

Index: clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
===
--- /dev/null
+++ clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct Incomplete; // expected-note 4{{forward declaration of 'Incomplete'}}
+Incomplete f(Incomplete) = delete; // well-formed
+Incomplete g(Incomplete) {}// expected-error{{incomplete result type 'Incomplete' in function definition}}\
+// expected-error{{variable has incomplete type 'Incomplete'}}
+
+struct C {
+  Incomplete f(Incomplete) = delete; // well-formed
+  Incomplete g(Incomplete) {}// expected-error{{incomplete result type 'Incomplete' in function definition}}\
+  // expected-error{{variable has incomplete type 'Incomplete'}}
+};
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14185,7 +14185,7 @@
 Decl *
 Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Declarator ,
   MultiTemplateParamsArg TemplateParameterLists,
-  SkipBodyInfo *SkipBody) {
+  SkipBodyInfo *SkipBody, bool FnDeleted) {
   assert(getCurFunctionDecl() == nullptr && "Function parsing confused");
   assert(D.isFunctionDeclarator() && "Not a function declarator!");
   Scope *ParentScope = FnBodyScope->getParent();
@@ -14204,7 +14204,7 @@
 
   D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
-  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody);
+  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody, FnDeleted);
 
   if (!Bases.empty())
 ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(Dcl, Bases);
@@ -14380,7 +14380,7 @@
 }
 
 Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
-SkipBodyInfo *SkipBody) {
+SkipBodyInfo *SkipBody, bool FnDeleted) {
   if (!D) {
 // Parsing the function declaration failed in some way. Push on a fake scope
 // anyway so we can try to parse the function body.
@@ -14470,10 +14470,11 @@
   }
 
   // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // unless the function is deleted
+  // (C99 6.9.1p3, C++ [dcl.fct.def.general]p2).
   QualType ResultType = FD->getReturnType();
   if (!ResultType->isDependentType() && !ResultType->isVoidType() &&
-  !FD->isInvalidDecl() &&
+  !FD->isInvalidDecl() && !FnDeleted &&
   RequireCompleteType(FD->getLocation(), ResultType,
   diag::err_func_def_incomplete_result))
 FD->setInvalidDecl();
@@ -14482,8 +14483,9 @@
 PushDeclContext(FnBodyScope, FD);
 
   // Check the validity of our function parameters
-  CheckParmsForFunctionDef(FD->parameters(),
-   /*CheckParameterNames=*/true);
+  if (!FnDeleted)
+CheckParmsForFunctionDef(FD->parameters(),
+ /*CheckParameterNames=*/true);
 
   // Add non-parameter declarations already in the function to the current
   // scope.
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1302,11 +1302,13 @@
   // Tell the actions module that we have entered a function definition with the
   // specified Declarator for the function.
   Sema::SkipBodyInfo SkipBody;
+  bool Delete =
+  Tok.is(tok::equal) && NextToken().is(tok::kw_delete) ? true : false;
   Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,
   TemplateInfo.TemplateParams
   ? *TemplateInfo.TemplateParams
   : MultiTemplateParamsArg(),
-  );
+  , Delete);
 
   if (SkipBody.ShouldSkip) {
 SkipFunctionBody();
@@ -1332,7 +1334,6 @@
   if (TryConsumeToken(tok::equal)) {
 assert(getLangOpts().CPlusPlus && "Only C++ function definitions have '='");
 
-bool Delete = false;
 SourceLocation KWLoc;
 if (TryConsumeToken(tok::kw_delete, KWLoc)) {
   

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



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

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:1339
 
 bool Delete = false;
 SourceLocation KWLoc;

I'm thinking should we merge `FnDeleted` and `Deleted` into one variable or 
leave it as is? On the one hand, `Deleted`'s scope is tight so very readable. 
On the other hand, it seems redundant.



Comment at: clang/lib/Parse/Parser.cpp:1346
 << 1 /* deleted */;
   Actions.SetDeclDeleted(Res, KWLoc);
   Delete = true;

`FunctionDecl::setDeletedAsWritten` not called until this `SetDeclDeleted` call.



Comment at: clang/lib/Sema/SemaDecl.cpp:14457
   // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // unless the function is deleted
+  // (C99 6.9.1p3, C++ [dcl.fct.def.general]p2).

ChuanqiXu wrote:
> 
I think the sentence is not yet complete there so I mimic the old one on the 
left.



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14457
   // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // unless the function is deleted
+  // (C99 6.9.1p3, C++ [dcl.fct.def.general]p2).





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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao created this revision.
rZhBoYao added reviewers: ChuanqiXu, rsmith.
rZhBoYao added a project: clang.
Herald added a project: All.
rZhBoYao requested review of this revision.
Herald added a subscriber: cfe-commits.

According to dcl.fct.def.general p2 
 and this 
,
 deleted functions with incomplete return types and parameter types are 
well-formed.

  struct Incomplete;   // expected-note{{forward declaration of 
'Incomplete'}}
  Incomplete f() = delete; // well-formed
  Incomplete g(Incomplete a) = delete; // well-formed
  Incomplete h() {}// expected-error{{incomplete result 
type 'Incomplete' in function definition}}

Inspired by this tweet 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122981

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp

Index: clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
===
--- /dev/null
+++ clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct Incomplete;   // expected-note{{forward declaration of 'Incomplete'}}
+Incomplete f() = delete; // well-formed
+Incomplete g(Incomplete a) = delete; // well-formed
+Incomplete h() {}// expected-error{{incomplete result type 'Incomplete' in function definition}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14169,7 +14169,7 @@
 Decl *
 Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Declarator ,
   MultiTemplateParamsArg TemplateParameterLists,
-  SkipBodyInfo *SkipBody) {
+  SkipBodyInfo *SkipBody, bool FnDeleted) {
   assert(getCurFunctionDecl() == nullptr && "Function parsing confused");
   assert(D.isFunctionDeclarator() && "Not a function declarator!");
   Scope *ParentScope = FnBodyScope->getParent();
@@ -14188,7 +14188,7 @@
 
   D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
-  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody);
+  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody, FnDeleted);
 
   if (!Bases.empty())
 ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(Dcl, Bases);
@@ -14364,7 +14364,7 @@
 }
 
 Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
-SkipBodyInfo *SkipBody) {
+SkipBodyInfo *SkipBody, bool FnDeleted) {
   if (!D) {
 // Parsing the function declaration failed in some way. Push on a fake scope
 // anyway so we can try to parse the function body.
@@ -14454,10 +14454,11 @@
   }
 
   // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // unless the function is deleted
+  // (C99 6.9.1p3, C++ [dcl.fct.def.general]p2).
   QualType ResultType = FD->getReturnType();
   if (!ResultType->isDependentType() && !ResultType->isVoidType() &&
-  !FD->isInvalidDecl() &&
+  !FD->isInvalidDecl() && !FnDeleted &&
   RequireCompleteType(FD->getLocation(), ResultType,
   diag::err_func_def_incomplete_result))
 FD->setInvalidDecl();
@@ -14465,9 +14466,10 @@
   if (FnBodyScope)
 PushDeclContext(FnBodyScope, FD);
 
-  // Check the validity of our function parameters
-  CheckParmsForFunctionDef(FD->parameters(),
-   /*CheckParameterNames=*/true);
+  if (!FnDeleted)
+// Check the validity of our function parameters
+CheckParmsForFunctionDef(FD->parameters(),
+ /*CheckParameterNames=*/true);
 
   // Add non-parameter declarations already in the function to the current
   // scope.
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1302,11 +1302,15 @@
   // Tell the actions module that we have entered a function definition with the
   // specified Declarator for the function.
   Sema::SkipBodyInfo SkipBody;
+  bool FnDeleted = GetLookAheadToken(0).getKind() == tok::equal &&
+   GetLookAheadToken(1).getKind() == tok::kw_delete
+   ? true
+   : false;
   Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,