diff mbox

C++ PATCH for c++/48322 (broken handling of variadic parms on multiple levels)

Message ID 4EC9967E.2000308@redhat.com
State New
Headers show

Commit Message

Jason Merrill Nov. 21, 2011, 12:08 a.m. UTC 
Under this bug, given something like

template <class... Ts>
struct A
{
   template <class... Us,
             class V = tuple<pair<Ts,Us>...> >
   static void f();
};

When we instantiate A, we partially instantiate f.  But since we don't 
have all the Us, we can't instantiate pair<Ts,Us>... at all yet.  Before 
this patch we incorrectly modified it so that it ended up being treated 
as pair<Us,Us>... when we get around to instantiating f.

This patch handles the partial instantiation issue by attaching the Ts 
pack to the expansion, so substituting {int} for Ts basically turns the 
expansion into pair<Ts,Us>...[with Ts={int}].

It took me quite a few iterations to get the test right for when we want 
to do that, and when we want to substitute directly, but I think I got 
there in the end.

Tested x86_64-pc-linux-gnu, applying to trunk.

Comments

Jason Merrill Nov. 21, 2011, 12:22 a.m. UTC | #1 
This bug fix required the following change to 
libstdc++-v3/include/std/tuple:

> -      template<typename... _UElements, typename = typename
> -       enable_if<__and_<integral_constant<bool, sizeof...(_UElements)
> -                                          == sizeof...(_Elements)>,
> -                        __all_convertible<__conv_types<_UElements...>,
> -                                          __conv_types<_Elements...>>
> -                        >::value>::type>
> +      template<typename... _UElements,
> +              typename = typename enable_if<sizeof...(_UElements)
> +                                            == sizeof...(_Elements)>::type,
> +              typename = typename
> +                enable_if<__all_convertible<__conv_types<_UElements...>,
> +                                            __conv_types<_Elements...> >::value
> +                          >::type>

__and_ doesn't have the shortcut semantics of &&, so checking for equal 
sizeof... didn't prevent us from trying the __all_convertible test.

Hmm, actually, that still shouldn't have produced an error, it should 
just SFINAE.  Another bug to fix...

Jason
Paolo Carlini Nov. 21, 2011, 12:35 a.m. UTC | #2 
On 11/21/2011 01:22 AM, Jason Merrill wrote:
> This bug fix required the following change to 
> libstdc++-v3/include/std/tuple:
>
>> -      template<typename... _UElements, typename = typename
>> -       enable_if<__and_<integral_constant<bool, sizeof...(_UElements)
>> -                                          == sizeof...(_Elements)>,
>> -                        __all_convertible<__conv_types<_UElements...>,
>> -                                          __conv_types<_Elements...>>
>> - >::value>::type>
>> +      template<typename... _UElements,
>> +              typename = typename enable_if<sizeof...(_UElements)
>> +                                            == 
>> sizeof...(_Elements)>::type,
>> +              typename = typename
>> +                
>> enable_if<__all_convertible<__conv_types<_UElements...>,
>> +                                            
>> __conv_types<_Elements...> >::value
>> + >::type>
>
> __and_ doesn't have the shortcut semantics of &&, so checking for 
> equal sizeof... didn't prevent us from trying the __all_convertible test.
Ah!! So, now I'm puzzled: why are we using all those __and_ and __or_ in 
the first place, if isn't because we want to avoid to avoid some 
instantiations? Daniel can you remind me that?

Thanks,
Paolo.
Paolo Carlini Nov. 21, 2011, 1:02 a.m. UTC | #3 
.. also, I was under the impression that c++/48322 was the only reason 
we couldn't write something very simple, thus no __all_convertible, ie, 
something using directly:

     enable_if<__and_<is_convertible<_UElements, 
_Elements>...>::value>::type

That would be a great improvement, even if we need to SFINAE separately 
for equal sizeofs. But maybe it works, have just to clean up the code?!

Paolo.
Jason Merrill Nov. 21, 2011, 2:05 a.m. UTC | #4 
On 11/20/2011 08:02 PM, Paolo Carlini wrote:
> .. also, I was under the impression that c++/48322 was the only reason
> we couldn't write something very simple, thus no __all_convertible, ie,
> something using directly:
>
> enable_if<__and_<is_convertible<_UElements, _Elements>...>::value>::type

Yep, you can do that now.

> That would be a great improvement, even if we need to SFINAE separately
> for equal sizeofs.

You don't need to, you can just drop the sizeof check now.

Jason
Daniel Krügler Nov. 21, 2011, 6:49 a.m. UTC | #5 
2011/11/21 Jason Merrill <jason@redhat.com>:
> On 11/20/2011 08:02 PM, Paolo Carlini wrote:
>>
>> .. also, I was under the impression that c++/48322 was the only reason
>> we couldn't write something very simple, thus no __all_convertible, ie,
>> something using directly:
>>
>> enable_if<__and_<is_convertible<_UElements, _Elements>...>::value>::type
>
> Yep, you can do that now.

Yes, __all_convertible is a relict that made sense, before __and_
was introduced. I'm using now the

__and_<is_something<_UElements, _Elements>...>

expansion idiom regularly.

Daniel
Daniel Krügler Nov. 21, 2011, 6:53 a.m. UTC | #6 
2011/11/21 Paolo Carlini <paolo.carlini@oracle.com>:
> On 11/21/2011 01:22 AM, Jason Merrill wrote:
>>
>> This bug fix required the following change to
>> libstdc++-v3/include/std/tuple:
>>
>>> -      template<typename... _UElements, typename = typename
>>> -       enable_if<__and_<integral_constant<bool, sizeof...(_UElements)
>>> -                                          == sizeof...(_Elements)>,
>>> -                        __all_convertible<__conv_types<_UElements...>,
>>> -                                          __conv_types<_Elements...>>
>>> - >::value>::type>
>>> +      template<typename... _UElements,
>>> +              typename = typename enable_if<sizeof...(_UElements)
>>> +                                            ==
>>> sizeof...(_Elements)>::type,
>>> +              typename = typename
>>> +                enable_if<__all_convertible<__conv_types<_UElements...>,
>>> +                                            __conv_types<_Elements...>
>>> >::value
>>> + >::type>
>>
>> __and_ doesn't have the shortcut semantics of &&, so checking for equal
>> sizeof... didn't prevent us from trying the __all_convertible test.
>
> Ah!! So, now I'm puzzled: why are we using all those __and_ and __or_ in the
> first place, if isn't because we want to avoid to avoid some instantiations?
> Daniel can you remind me that?

AFAIK in my actual __all_convertible implementation, the second
test was protected by lazy __and_ usage, so this could not happen.
I assume that the defect occurred, when you adapted it to the
existing structure. But of-course the more general

__and_<is_something<_UElements, _Elements>...>

expansion is preferred now.

- Daniel
Daniel Krügler Nov. 21, 2011, 7:22 a.m. UTC | #7 
2011/11/21 Daniel Krügler <daniel.kruegler@googlemail.com>:
> 2011/11/21 Jason Merrill <jason@redhat.com>:
>> On 11/20/2011 08:02 PM, Paolo Carlini wrote:
>>>
>>> .. also, I was under the impression that c++/48322 was the only reason
>>> we couldn't write something very simple, thus no __all_convertible, ie,
>>> something using directly:
>>>
>>> enable_if<__and_<is_convertible<_UElements, _Elements>...>::value>::type
>>
>> Yep, you can do that now.
>
> Yes, __all_convertible is a relict that made sense, before __and_
> was introduced.

To be more precise here: The direct expansion did raise compiler
errors at that time, which seem to be fixed now.

- Daniel
diff mbox

Patch

commit fd65a2757ef89f9079b28b6a3296161a8202fe36
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Nov 18 23:24:56 2011 -0500

    	PR c++/48322
    gcc/cp/
    	* cp-tree.h (PACK_EXPANSION_EXTRA_ARGS): New.
    	* cp-tree.def (EXPR_PACK_EXPANSION): Add an operand for it.
    	* pt.c (tsubst_pack_expansion): Set and use it.
    	(iterative_hash_template_arg): Hash it.
    	(template_args_equal): Compare it.
    	(comp_template_args_with_info): Handle nulls.
    	* tree.c (cp_walk_subtrees): Walk it.
    	* typeck.c (structural_comptypes): Compare it.
    	* ptree.c (cxx_print_type): Print it.
    libstdc++-v3/
    	* include/std/tuple (tuple(_UElements&&...)): Fix SFINAE.

diff --git a/gcc/cp/cp-tree.def b/gcc/cp/cp-tree.def
index 4eec9f9..5fc5496 100644
--- a/gcc/cp/cp-tree.def
+++ b/gcc/cp/cp-tree.def
@@ -419,7 +419,7 @@  DEFTREECODE (TYPE_PACK_EXPANSION, "type_pack_expansion", tcc_type, 0)
 
    EXPR_PACK_EXPANSION plays precisely the same role as TYPE_PACK_EXPANSION,
    but will be used for expressions.  */
-DEFTREECODE (EXPR_PACK_EXPANSION, "expr_pack_expansion", tcc_expression, 2)
+DEFTREECODE (EXPR_PACK_EXPANSION, "expr_pack_expansion", tcc_expression, 3)
 
 /* Selects the Ith parameter out of an argument pack. This node will
    be used when instantiating pack expansions; see
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index fe50e34..3f4f408 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -2813,7 +2813,14 @@  extern void decl_shadowed_for_var_insert (tree, tree);
 #define PACK_EXPANSION_PARAMETER_PACKS(NODE)		\
   *(TREE_CODE (NODE) == EXPR_PACK_EXPANSION		\
     ? &TREE_OPERAND (NODE, 1)				\
-    : &TREE_CHAIN (TYPE_PACK_EXPANSION_CHECK (NODE)))
+    : &TYPE_MINVAL (TYPE_PACK_EXPANSION_CHECK (NODE)))
+
+/* Any additional template args to be applied when substituting into
+   the pattern, set by tsubst_pack_expansion for partial instantiations.  */
+#define PACK_EXPANSION_EXTRA_ARGS(NODE)		\
+  *(TREE_CODE (NODE) == TYPE_PACK_EXPANSION	\
+    ? &TYPE_MAXVAL (NODE)			\
+    : &TREE_OPERAND ((NODE), 2))
 
 /* Determine if this is an argument pack.  */
 #define ARGUMENT_PACK_P(NODE)                          \
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 3232527..5e8120b 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -1534,7 +1534,8 @@  iterative_hash_template_arg (tree arg, hashval_t val)
 
     case TYPE_PACK_EXPANSION:
     case EXPR_PACK_EXPANSION:
-      return iterative_hash_template_arg (PACK_EXPANSION_PATTERN (arg), val);
+      val = iterative_hash_template_arg (PACK_EXPANSION_PATTERN (arg), val);
+      return iterative_hash_template_arg (PACK_EXPANSION_EXTRA_ARGS (arg), val);
 
     case TYPE_ARGUMENT_PACK:
     case NONTYPE_ARGUMENT_PACK:
@@ -6902,9 +6903,11 @@  template_args_equal (tree ot, tree nt)
     /* For member templates */
     return TREE_CODE (ot) == TREE_VEC && comp_template_args (ot, nt);
   else if (PACK_EXPANSION_P (ot))
-    return PACK_EXPANSION_P (nt) 
-      && template_args_equal (PACK_EXPANSION_PATTERN (ot),
-                              PACK_EXPANSION_PATTERN (nt));
+    return (PACK_EXPANSION_P (nt)
+	    && template_args_equal (PACK_EXPANSION_PATTERN (ot),
+				    PACK_EXPANSION_PATTERN (nt))
+	    && template_args_equal (PACK_EXPANSION_EXTRA_ARGS (ot),
+				    PACK_EXPANSION_EXTRA_ARGS (nt)));
   else if (ARGUMENT_PACK_P (ot))
     {
       int i, len;
@@ -6954,6 +6957,12 @@  comp_template_args_with_info (tree oldargs, tree newargs,
 {
   int i;
 
+  if (oldargs == newargs)
+    return 1;
+
+  if (!oldargs || !newargs)
+    return 0;
+
   if (TREE_VEC_LENGTH (oldargs) != TREE_VEC_LENGTH (newargs))
     return 0;
 
@@ -9241,13 +9250,21 @@  tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
   tree pattern;
   tree pack, packs = NULL_TREE;
   bool unsubstituted_packs = false;
+  bool real_packs = false;
+  int missing_level = 0;
   int i, len = -1;
   tree result;
   htab_t saved_local_specializations = NULL;
+  int levels;
 
   gcc_assert (PACK_EXPANSION_P (t));
   pattern = PACK_EXPANSION_PATTERN (t);
 
+  /* Add in any args remembered from an earlier partial instantiation.  */
+  args = add_to_template_args (PACK_EXPANSION_EXTRA_ARGS (t), args);
+
+  levels = TMPL_ARGS_DEPTH (args);
+
   /* Determine the argument packs that will instantiate the parameter
      packs used in the expansion expression. While we're at it,
      compute the number of arguments to be expanded and make sure it
@@ -9258,6 +9275,7 @@  tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
       tree parm_pack = TREE_VALUE (pack);
       tree arg_pack = NULL_TREE;
       tree orig_arg = NULL_TREE;
+      int level = 0;
 
       if (TREE_CODE (parm_pack) == BASES)
        {
@@ -9290,10 +9308,9 @@  tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
 	}
       else
         {
-          int level, idx, levels;
+	  int idx;
           template_parm_level_and_index (parm_pack, &level, &idx);
 
-          levels = TMPL_ARGS_DEPTH (args);
           if (level <= levels)
             arg_pack = TMPL_ARG (args, level, idx);
         }
@@ -9344,6 +9361,13 @@  tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
               return error_mark_node;
             }
 
+	  if (TREE_VEC_LENGTH (ARGUMENT_PACK_ARGS (arg_pack)) == 1
+	      && PACK_EXPANSION_P (TREE_VEC_ELT (ARGUMENT_PACK_ARGS (arg_pack),
+						 0)))
+	    /* This isn't a real argument pack yet.  */;
+	  else
+	    real_packs = true;
+
           /* Keep track of the parameter packs and their corresponding
              argument packs.  */
           packs = tree_cons (parm_pack, arg_pack, packs);
@@ -9351,25 +9375,57 @@  tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
         }
       else
 	{
-	  /* We can't substitute for this parameter pack.  */
+	  /* We can't substitute for this parameter pack.  We use a flag as
+	     well as the missing_level counter because function parameter
+	     packs don't have a level.  */
 	  unsubstituted_packs = true;
-	  break;
+	  if (!missing_level || missing_level > level)
+	    missing_level = level;
 	}
     }
 
   /* We cannot expand this expansion expression, because we don't have
-     all of the argument packs we need. Substitute into the pattern
-     and return a PACK_EXPANSION_*. The caller will need to deal with
-     that.  */
+     all of the argument packs we need.  */
   if (unsubstituted_packs)
     {
-      tree new_pat;
-      if (TREE_CODE (t) == EXPR_PACK_EXPANSION)
-	new_pat = tsubst_expr (pattern, args, complain, in_decl,
-			       /*integral_constant_expression_p=*/false);
+      if (real_packs)
+	{
+	  /* We got some full packs, but we can't substitute them in until we
+	     have values for all the packs.  So remember these until then.  */
+	  tree save_args;
+
+	  t = make_pack_expansion (pattern);
+
+	  /* The call to add_to_template_args above assumes no overlap
+	     between saved args and new args, so prune away any fake
+	     args, i.e. those that satisfied arg_from_parm_pack_p above.  */
+	  if (missing_level && levels >= missing_level)
+	    {
+	      gcc_assert (TMPL_ARGS_HAVE_MULTIPLE_LEVELS (args)
+			  && missing_level > 1);
+	      TREE_VEC_LENGTH (args) = missing_level - 1;
+	      save_args = copy_node (args);
+	      TREE_VEC_LENGTH (args) = levels;
+	    }
+	  else
+	    save_args = args;
+
+	  PACK_EXPANSION_EXTRA_ARGS (t) = save_args;
+	}
       else
-	new_pat = tsubst (pattern, args, complain, in_decl);
-      return make_pack_expansion (new_pat);
+	{
+	  /* There were no real arguments, we're just replacing a parameter
+	     pack with another version of itself. Substitute into the
+	     pattern and return a PACK_EXPANSION_*. The caller will need to
+	     deal with that.  */
+	  if (TREE_CODE (t) == EXPR_PACK_EXPANSION)
+	    t = tsubst_expr (pattern, args, complain, in_decl,
+			     /*integral_constant_expression_p=*/false);
+	  else
+	    t = tsubst (pattern, args, complain, in_decl);
+	  t = make_pack_expansion (t);
+	}
+      return t;
     }
 
   /* We could not find any argument packs that work.  */
diff --git a/gcc/cp/ptree.c b/gcc/cp/ptree.c
index fb05e13..a66e695 100644
--- a/gcc/cp/ptree.c
+++ b/gcc/cp/ptree.c
@@ -104,6 +104,10 @@  cxx_print_type (FILE *file, tree node, int indent)
 		  indent + 4);
       return;
 
+    case TYPE_PACK_EXPANSION:
+      print_node (file, "args", PACK_EXPANSION_EXTRA_ARGS (node), indent + 4);
+      return;
+
     default:
       return;
     }
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 841029f..d206fd2 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -2993,11 +2993,13 @@  cp_walk_subtrees (tree *tp, int *walk_subtrees_p, walk_tree_fn func,
 
     case TYPE_PACK_EXPANSION:
       WALK_SUBTREE (TREE_TYPE (*tp));
+      WALK_SUBTREE (PACK_EXPANSION_EXTRA_ARGS (*tp));
       *walk_subtrees_p = 0;
       break;
       
     case EXPR_PACK_EXPANSION:
       WALK_SUBTREE (TREE_OPERAND (*tp, 0));
+      WALK_SUBTREE (PACK_EXPANSION_EXTRA_ARGS (*tp));
       *walk_subtrees_p = 0;
       break;
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index b8d4c10..a23e274 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1329,8 +1329,10 @@  structural_comptypes (tree t1, tree t2, int strict)
       break;
 
     case TYPE_PACK_EXPANSION:
-      return same_type_p (PACK_EXPANSION_PATTERN (t1), 
-                          PACK_EXPANSION_PATTERN (t2));
+      return (same_type_p (PACK_EXPANSION_PATTERN (t1),
+			   PACK_EXPANSION_PATTERN (t2))
+	      && comp_template_args (PACK_EXPANSION_EXTRA_ARGS (t1),
+				     PACK_EXPANSION_EXTRA_ARGS (t2)));
 
     case DECLTYPE_TYPE:
       if (DECLTYPE_TYPE_ID_EXPR_OR_MEMBER_ACCESS_P (t1)
diff --git a/gcc/testsuite/g++.dg/cpp0x/sfinae26.C b/gcc/testsuite/g++.dg/cpp0x/sfinae26.C
index 42b48eb..374f997 100644
--- a/gcc/testsuite/g++.dg/cpp0x/sfinae26.C
+++ b/gcc/testsuite/g++.dg/cpp0x/sfinae26.C
@@ -30,9 +30,9 @@  struct is_same<T, T> {
 template<class... T>
 struct S {
   template<class... U,
-    typename enable_if<and_<is_same<T, U>...>::value>::type*& = enabler
+    typename enable_if<and_<is_same<T, U>...>::value>::type*& = enabler // { dg-error "no type" }
   >
-  S(U...){}			// { dg-error "no type named 'type'" }
+  S(U...){}
 };
 
 S<bool> s(0);			// { dg-error "no match" }
diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic120.C b/gcc/testsuite/g++.dg/cpp0x/variadic120.C
new file mode 100644
index 0000000..e26ee4e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/variadic120.C
@@ -0,0 +1,24 @@ 
+// PR c++/48322
+// { dg-do compile { target c++11 } }
+
+template <class... T> struct tuple;
+template <class T> struct tuple<T> { T t; };
+
+template <class T, class U> struct pair;
+template<> struct pair<int,double> { };
+
+template <class... Ts>
+struct A
+{
+  template <class... Us,
+            class V = tuple<pair<Ts,Us>...> >
+  static void f()
+  {
+    V v;
+  }
+};
+
+int main()
+{
+  A<int>::f<double>();
+}
diff --git a/libstdc++-v3/include/std/tuple b/libstdc++-v3/include/std/tuple
index 51be289..474634f 100644
--- a/libstdc++-v3/include/std/tuple
+++ b/libstdc++-v3/include/std/tuple
@@ -407,12 +407,13 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       constexpr tuple(const _Elements&... __elements)
       : _Inherited(__elements...) { }
 
-      template<typename... _UElements, typename = typename
-	enable_if<__and_<integral_constant<bool, sizeof...(_UElements)
-					   == sizeof...(_Elements)>,
-			 __all_convertible<__conv_types<_UElements...>,
-					   __conv_types<_Elements...>>
-			 >::value>::type>
+      template<typename... _UElements,
+	       typename = typename enable_if<sizeof...(_UElements)
+					     == sizeof...(_Elements)>::type,
+	       typename = typename
+	         enable_if<__all_convertible<__conv_types<_UElements...>,
+					     __conv_types<_Elements...> >::value
+			   >::type>
 	explicit
         constexpr tuple(_UElements&&... __elements)
 	: _Inherited(std::forward<_UElements>(__elements)...) {	}