[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D138210#3989385 , @luken-google 
wrote:

> committed at afd800fc5679ccfe6f32b097586c658628500867 
> 

For future reference it's handy to include the "Differential Revision: 
https://reviews.llvm.org/D; (`arc` will create this for you, if you use 
that) in the commit message both so folks looking at the commit can find the 
review/know that it was reviewed on Phab, and that way Phab will auto-close the 
review and include a link to the commit.

Also I think we're still using the "PR" prefix predominantly for bug 
references, rather than the "GH" suffix you've used here (handy for consistency 
- easier to search up the bugs if the prefix is more consistent).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google closed this revision.
luken-google added a comment.

committed at afd800fc5679ccfe6f32b097586c658628500867 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the explanation. LGTM! And thanks for adding an assert.

It's interesting that recovery for classes seems 
 to be a bit better here:

  template  struct invalid {};
  int a = invalid(10); // there is no error: undefined identified 
'invalid' 

but I suspect chasing the improvements there is out of scope for this 
particular GH issue. Maybe worth adding a FIXME/filing a GH issue for improving 
this in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 478019.
luken-google added a comment.

Strengthen assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template 
parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared 
identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared 
identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' 
after expression}}
+   // expected-error@-1{{use of 
undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected 
';' after expression}}
+   // expected-error@-3{{use of 
undeclared identifier 'just'}}
+   // expected-error@-4{{unknown 
type name 'in'}}
+return b;
+}
+} // namespace GH48182
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -5981,8 +5981,12 @@
   // A non-expanded parameter pack before the end of the parameter list
   // only occurs for an ill-formed template parameter list, unless we've
   // got a partial argument list for a function template, so just bail out.
-  if (Param + 1 != ParamEnd)
+  if (Param + 1 != ParamEnd) {
+assert(
+(Template->getMostRecentDecl()->getKind() != Decl::Kind::Concept) 
&&
+"Concept templates must have parameter packs at the end.");
 return true;
+  }
 
   SugaredConverted.push_back(
   TemplateArgument::CreatePackCopy(Context, SugaredArgumentPack));
@@ -8922,17 +8926,33 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  // Ensure that the parameter pack, if present, is the last parameter in the
+  // template.
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param = *ParamIt;
+if (Param->isParameterPack()) {
+  if (++ParamIt == ParamEnd)
+break;
+  Diag(Param->getLocation(),
+   diag::err_template_param_pack_must_be_last_template_parameter);
+  return nullptr;
+}
+  }
+
   if (DiagnoseUnexpandedParameterPack(ConstraintExpr))
 return nullptr;
 
-  ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, NameLoc, Name,
- TemplateParameterLists.front(),
- ConstraintExpr);
+  ConceptDecl *NewDecl =
+  ConceptDecl::Create(Context, DC, NameLoc, Name, Params, ConstraintExpr);
 
   if (NewDecl->hasAssociatedConstraints()) {
 // C++2a [temp.concept]p4:
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -648,6 +648,8 @@
   ([temp.func.order]p6.2.1 is not implemented, matching GCC).
 - Implemented `P0857R0 
`_,
   which specifies constrained lambdas and constrained template 
*template-parameter*\s.
+- Required parameter pack to be provided at the end of the concept parameter 
list. This
+  fixes `Issue 48182 `_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 
`_.


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template parameter pack must be the last template parameter}}
+concept 

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

There is logic in the template expansion that returns an error state if the 
template parameter pack was not at the end of the template parameter list. It 
returns `true` but does not generate any diagnostics, but the outer code error 
machinery assumes that diagnostics have already been generated and so considers 
the expression invalid and moves on.

As this fix no longer accepts these ill-formed templates as valid, that code no 
longer fires. I have strengthened the check there to an assert. PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

E.g. gcc actually accepts the concept definition, but later fails with "wrong 
number of template arguments". I wonder why Clang has not been doing the latter 
before this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The error itself makes sense, but I don't quite understand why usages of 
`invalid` did not produce errors before. Any idea why that happened? Are 
there some other bugs hiding?
It seems that at some point when parsing this code:

  static_assert(invalid also here ;

we chose to silently recover by skipping until the semicolon, but never 
actually produced any errors. Is there a code path that should also be updated 
to handle that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 476462.
luken-google added a comment.

Adding release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template 
parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared 
identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared 
identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' 
after expression}}
+   // expected-error@-1{{use of 
undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected 
';' after expression}}
+   // expected-error@-3{{use of 
undeclared identifier 'just'}}
+   // expected-error@-4{{unknown 
type name 'in'}}
+return b;
+}
+} // namespace GH48182
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8923,17 +8923,31 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param = *ParamIt;
+if (Param->isParameterPack()) {
+  if (++ParamIt == ParamEnd)
+break;
+  Diag(Param->getLocation(),
+   diag::err_template_param_pack_must_be_last_template_parameter);
+  return nullptr;
+}
+  }
+
   if (DiagnoseUnexpandedParameterPack(ConstraintExpr))
 return nullptr;
 
-  ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, NameLoc, Name,
- TemplateParameterLists.front(),
- ConstraintExpr);
+  ConceptDecl *NewDecl =
+  ConceptDecl::Create(Context, DC, NameLoc, Name, Params, ConstraintExpr);
 
   if (NewDecl->hasAssociatedConstraints()) {
 // C++2a [temp.concept]p4:
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -638,6 +638,8 @@
   ([temp.func.order]p6.2.1 is not implemented, matching GCC).
 - Implemented `P0857R0 
`_,
   which specifies constrained lambdas and constrained template 
*template-parameter*\s.
+- Required parameter pack to be provided at the end of the concept parameter 
list. This
+  fixes `Issue 48182 `_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 
`_.


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' after expression}}
+   // expected-error@-1{{use of undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected ';' after expression}}
+   // expected-error@-3{{use of undeclared identifier 'just'}}
+   // 

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google created this revision.
Herald added a project: All.
luken-google requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes GH48182.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138210

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template 
parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared 
identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared 
identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' 
after expression}}
+   // expected-error@-1{{use of 
undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected 
';' after expression}}
+   // expected-error@-3{{use of 
undeclared identifier 'just'}}
+   // expected-error@-4{{unknown 
type name 'in'}}
+return b;
+}
+} // namespace GH48182
\ No newline at end of file
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8923,17 +8923,31 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param = *ParamIt;
+if (Param->isParameterPack()) {
+  if (++ParamIt == ParamEnd)
+break;
+  Diag(Param->getLocation(),
+   diag::err_template_param_pack_must_be_last_template_parameter);
+  return nullptr;
+}
+  }
+
   if (DiagnoseUnexpandedParameterPack(ConstraintExpr))
 return nullptr;
 
-  ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, NameLoc, Name,
- TemplateParameterLists.front(),
- ConstraintExpr);
+  ConceptDecl *NewDecl =
+  ConceptDecl::Create(Context, DC, NameLoc, Name, Params, ConstraintExpr);
 
   if (NewDecl->hasAssociatedConstraints()) {
 // C++2a [temp.concept]p4:


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' after expression}}
+   // expected-error@-1{{use of undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected ';' after expression}}
+   // expected-error@-3{{use of undeclared identifier 'just'}}
+   // expected-error@-4{{unknown type name 'in'}}
+return b;
+}
+} // namespace GH48182
\ No newline at end of file
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8923,17 +8923,31 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param =