[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner updated this revision to Diff 265683.
cpplearner added a comment.

rebase & ping @rsmith


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

https://reviews.llvm.org/D65050

Files:
  clang/lib/AST/Type.cpp
  clang/test/SemaTemplate/alias-templates.cpp


Index: clang/test/SemaTemplate/alias-templates.cpp
===
--- clang/test/SemaTemplate/alias-templates.cpp
+++ clang/test/SemaTemplate/alias-templates.cpp
@@ -265,3 +265,34 @@
 int z = Bar(); // expected-error {{use of template template parameter 
'Bar' requires template arguments}}
   }
 }
+
+namespace PR42654 {
+  template struct function { };
+
+  template
+  struct thing {
+void f(function) { }
+  };
+
+  template
+  struct Environment
+  {
+template
+using Integer = int;
+
+using Function = function...)>;
+using MyTuple = thing...>;
+
+void run(Function func)
+{
+  MyTuple t;
+  t.f(func);
+}
+  };
+
+  void f()
+  {
+Environment env;
+env.run({});
+  }
+}
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -3110,6 +3110,11 @@
   for (unsigned i = 0; i != getNumParams(); ++i) {
 addDependence(params[i]->getDependence() &
   ~TypeDependence::VariablyModified);
+// A pack expansion with a non-dependent pattern affects the number of
+// parameters, thus we mark such function type as dependent, even though
+// this isn't listed in N4861 [temp.dep.type].
+if (params[i]->getAs())
+  addDependence(TypeDependence::Dependent);
 argSlot[i] = params[i];
   }
 


Index: clang/test/SemaTemplate/alias-templates.cpp
===
--- clang/test/SemaTemplate/alias-templates.cpp
+++ clang/test/SemaTemplate/alias-templates.cpp
@@ -265,3 +265,34 @@
 int z = Bar(); // expected-error {{use of template template parameter 'Bar' requires template arguments}}
   }
 }
+
+namespace PR42654 {
+  template struct function { };
+
+  template
+  struct thing {
+void f(function) { }
+  };
+
+  template
+  struct Environment
+  {
+template
+using Integer = int;
+
+using Function = function...)>;
+using MyTuple = thing...>;
+
+void run(Function func)
+{
+  MyTuple t;
+  t.f(func);
+}
+  };
+
+  void f()
+  {
+Environment env;
+env.run({});
+  }
+}
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -3110,6 +3110,11 @@
   for (unsigned i = 0; i != getNumParams(); ++i) {
 addDependence(params[i]->getDependence() &
   ~TypeDependence::VariablyModified);
+// A pack expansion with a non-dependent pattern affects the number of
+// parameters, thus we mark such function type as dependent, even though
+// this isn't listed in N4861 [temp.dep.type].
+if (params[i]->getAs())
+  addDependence(TypeDependence::Dependent);
 argSlot[i] = params[i];
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

ping @rsmith


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

In D65050#1608777 , @efriedma wrote:

> > this looks like it could be a Core Issue
>
> I think the issue is clang-specific. clang splits the standard notion of a 
> dependent type into two separate bits, for the sake of diagnostics: 
> `isDependentType()`, and `isInstantiationDependentType()`.  
> `isInstantiationDependentType()` reflects the actual standard definition of a 
> dependent type; `isDependentType()` is a type that can actually vary across 
> instantiations.


According to comment in `include/clang/AST/Type.h` 
(https://github.com/llvm/llvm-project/blob/llvmorg-8.0.1/clang/include/clang/AST/Type.h#L1426),
 `Dependent` reflects the standard definition, while `InstantiationDependent` 
reflects "whether this type somehow involves a template parameter, even if the 
resolution of the type does not depend on a template parameter" (e.g. 
`decltype(sizeof(T))`).

Therefore, I agree with Aaron that this could be a Core issue.


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

ping @rsmith


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

ping


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> this looks like it could be a Core Issue

I think the issue is clang-specific. clang splits the standard notion of a 
dependent type into two separate bits, for the sake of diagnostics: 
`isDependentType()`, and `isInstantiationDependentType()`.  
`isInstantiationDependentType()` reflects the actual standard definition of a 
dependent type; `isDependentType()` is a type that can actually vary across 
instantiations.


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this is reasonable, but I'd like @rsmith to weigh in -- this looks like 
it could be a Core Issue.


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

In D65050#1606022 , @aaron.ballman 
wrote:

> The noexcept specifier is part of the type these days, is that also handled 
> properly?


I believe that it's properly handled in this section of 
`FunctionProtoType::FunctionProtoType`:

  // If this is a canonical type, and its exception specification is dependent,
  // then it's a dependent type. This only happens in C++17 onwards.
  if (isCanonicalUnqualified()) {
if (getExceptionSpecType() == EST_Dynamic ||
getExceptionSpecType() == EST_DependentNoexcept) {
  assert(hasDependentExceptionSpec() && "type should not be canonical");
  setDependent();
}
  } else if (getCanonicalTypeInternal()->isDependentType()) {
// Ask our canonical type whether our exception specification was dependent.
setDependent();
  }


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D65050#1598057 , @cpplearner wrote:

> In D65050#1596514 , @efriedma wrote:
>
> > Is this the only place where a compound type can contain a 
> > PackExpansionType?
>
>
> Yes AFAIK. No other place can contain a list or a pack expansion. The closest 
> thing is dynamic exception specification (which isn't part of the type), and 
> it seems to be handled properly.


The noexcept specifier is part of the type these days, is that also handled 
properly?


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

ping


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

In D65050#1596514 , @efriedma wrote:

> Is this the only place where a compound type can contain a PackExpansionType?


Yes AFAIK. No other place can contain a list or a pack expansion. The closest 
thing is dynamic exception specification (which isn't part of the type), and it 
seems to be handled properly.

> The code could probably use a brief comment explaining why we need to check 
> this explicitly, even though it isn't listed in [temp.dep.type].

Done.


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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner updated this revision to Diff 211358.

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

https://reviews.llvm.org/D65050

Files:
  clang/lib/AST/Type.cpp
  clang/test/SemaTemplate/alias-templates.cpp


Index: clang/test/SemaTemplate/alias-templates.cpp
===
--- clang/test/SemaTemplate/alias-templates.cpp
+++ clang/test/SemaTemplate/alias-templates.cpp
@@ -267,3 +267,34 @@
 int z = Bar(); // expected-error {{use of template template parameter 
'Bar' requires template arguments}}
   }
 }
+
+namespace PR42654 {
+  template struct function { };
+
+  template
+  struct thing {
+void f(function) { }
+  };
+
+  template
+  struct Environment
+  {
+template
+using Integer = int;
+
+using Function = function...)>;
+using MyTuple = thing...>;
+
+void run(Function func)
+{
+  MyTuple t;
+  t.f(func);
+}
+  };
+
+  void f()
+  {
+Environment env;
+env.run({});
+  }
+}
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -2912,7 +2912,10 @@
   // Fill in the trailing argument array.
   auto *argSlot = getTrailingObjects();
   for (unsigned i = 0; i != getNumParams(); ++i) {
-if (params[i]->isDependentType())
+// A pack expansion with a non-dependent pattern still affects the number 
of
+// parameters, thus we mark such function type as dependent, even though
+// this isn't listed in N4820 [temp.dep.type].
+if (params[i]->isDependentType() || params[i]->getAs())
   setDependent();
 else if (params[i]->isInstantiationDependentType())
   setInstantiationDependent();


Index: clang/test/SemaTemplate/alias-templates.cpp
===
--- clang/test/SemaTemplate/alias-templates.cpp
+++ clang/test/SemaTemplate/alias-templates.cpp
@@ -267,3 +267,34 @@
 int z = Bar(); // expected-error {{use of template template parameter 'Bar' requires template arguments}}
   }
 }
+
+namespace PR42654 {
+  template struct function { };
+
+  template
+  struct thing {
+void f(function) { }
+  };
+
+  template
+  struct Environment
+  {
+template
+using Integer = int;
+
+using Function = function...)>;
+using MyTuple = thing...>;
+
+void run(Function func)
+{
+  MyTuple t;
+  t.f(func);
+}
+  };
+
+  void f()
+  {
+Environment env;
+env.run({});
+  }
+}
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -2912,7 +2912,10 @@
   // Fill in the trailing argument array.
   auto *argSlot = getTrailingObjects();
   for (unsigned i = 0; i != getNumParams(); ++i) {
-if (params[i]->isDependentType())
+// A pack expansion with a non-dependent pattern still affects the number of
+// parameters, thus we mark such function type as dependent, even though
+// this isn't listed in N4820 [temp.dep.type].
+if (params[i]->isDependentType() || params[i]->getAs())
   setDependent();
 else if (params[i]->isInstantiationDependentType())
   setInstantiationDependent();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[Eli Friedman via Phabricator via cfe-commits]

efriedma edited reviewers, added: aaron.ballman; removed: doug.gregor, 
eli.friedman, lvoufo.
efriedma added a comment.

Is this the only place where a compound type can contain a PackExpansionType?

The code could probably use a brief comment explaining why we need to check 
this explicitly, even though it isn't listed in [temp.dep.type].


Repository:
  rC Clang

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

https://reviews.llvm.org/D65050



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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner created this revision.
cpplearner added reviewers: doug.gregor, eli.friedman, lvoufo, rsmith.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

Given `template using Int = int;`, the type `void(Int...)` should be 
treated as a dependent type, even though `Int` is not dependent.

This fixes https://bugs.llvm.org/show_bug.cgi?id=42654


Repository:
  rC Clang

https://reviews.llvm.org/D65050

Files:
  clang/lib/AST/Type.cpp
  clang/test/SemaTemplate/alias-templates.cpp


Index: clang/test/SemaTemplate/alias-templates.cpp
===
--- clang/test/SemaTemplate/alias-templates.cpp
+++ clang/test/SemaTemplate/alias-templates.cpp
@@ -267,3 +267,34 @@
 int z = Bar(); // expected-error {{use of template template parameter 
'Bar' requires template arguments}}
   }
 }
+
+namespace PR42654 {
+  template struct function { };
+
+  template
+  struct thing {
+void f(function) { }
+  };
+
+  template
+  struct Environment
+  {
+template
+using Integer = int;
+
+using Function = function...)>;
+using MyTuple = thing...>;
+
+void run(Function func)
+{
+  MyTuple t;
+  t.f(func);
+}
+  };
+
+  void f()
+  {
+Environment env;
+env.run({});
+  }
+}
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -2912,7 +2912,7 @@
   // Fill in the trailing argument array.
   auto *argSlot = getTrailingObjects();
   for (unsigned i = 0; i != getNumParams(); ++i) {
-if (params[i]->isDependentType())
+if (params[i]->isDependentType() || params[i]->getAs())
   setDependent();
 else if (params[i]->isInstantiationDependentType())
   setInstantiationDependent();


Index: clang/test/SemaTemplate/alias-templates.cpp
===
--- clang/test/SemaTemplate/alias-templates.cpp
+++ clang/test/SemaTemplate/alias-templates.cpp
@@ -267,3 +267,34 @@
 int z = Bar(); // expected-error {{use of template template parameter 'Bar' requires template arguments}}
   }
 }
+
+namespace PR42654 {
+  template struct function { };
+
+  template
+  struct thing {
+void f(function) { }
+  };
+
+  template
+  struct Environment
+  {
+template
+using Integer = int;
+
+using Function = function...)>;
+using MyTuple = thing...>;
+
+void run(Function func)
+{
+  MyTuple t;
+  t.f(func);
+}
+  };
+
+  void f()
+  {
+Environment env;
+env.run({});
+  }
+}
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -2912,7 +2912,7 @@
   // Fill in the trailing argument array.
   auto *argSlot = getTrailingObjects();
   for (unsigned i = 0; i != getNumParams(); ++i) {
-if (params[i]->isDependentType())
+if (params[i]->isDependentType() || params[i]->getAs())
   setDependent();
 else if (params[i]->isInstantiationDependentType())
   setInstantiationDependent();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits