[PATCH] recog: Use parameter packs for operator()

[Richard Sandiford]

Jonathan Wakely <jwakely@redhat.com> writes:
> +  template<typename ...Ts>
> +  rtx_insn *operator() (Ts... args...) const
>
> Why is this declared as a variadic template **and** a varargs function?
>
> I think the second ellipsis is wrong, it should be just:
>
> +  template<typename ...Ts>
> +  rtx_insn *operator() (Ts... args) const

Oops, yes.

> And this seems like a more direct way to say "a list of N rtx types"
> where N is sizeof...(args):
>
> diff --git a/gcc/recog.h b/gcc/recog.h
> index 0a71a02c4a9..fd22c58c92a 100644
> --- a/gcc/recog.h
> +++ b/gcc/recog.h
> @@ -294,10 +294,13 @@ struct insn_gen_fn
>  {
>    typedef void (*stored_funcptr) (void);
>  
> +  template<typename T> using rtx_t = rtx;
> +
>    template<typename ...Ts>
> -  rtx_insn *operator() (Ts... args...) const
> +  rtx_insn *operator() (Ts... args) const
>    {
> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);
> +    using funcptr = rtx_insn * (*) (rtx_t<Ts>...);
> +    return ((funcptr) func) (args...);
>    }
>  
>    // This is for compatibility of code that invokes functions like
>
> The rtx_t<T> alias is the type 'rtx' for any T. The pack expansion
> rtx_t<Ts>... is a list of rtx the same length as the pack Ts.

Yeah.  As mentioned on IRC, I'd originally done it like this, but didn't
like the ad-hoc type alias.  I can't remember what name I'd used, but the
problem in both cases is/was that the ad-hoc name looks odd when you're
used to seeing plain “rtx”.  It also felt weird to expose this kind of
internal, function-specific implementation detail at insn_gen_fn scope.

Using decltype also gave nicer error messages, e.g.:

    invalid conversion from ‘int’ to ‘rtx {aka rtx_def*}’

instead of:

    invalid conversion from ‘int’ to ‘insn_gen_fn::rtx_t<int> {aka rtx_def*}’

I think the latter is likely to confuse people when they see it
for the first time.

So in some ways I'd prefer to keep the decltype and just add a void cast
to suppress the warning.  (Seems odd to warn about expressions having no
effect in an unevaluated context.)

But how about the below instead?  Hopefully the alias name is mnemonic
enough.

> The 'funcptr' alias sin't necessary, but (IMHO) simplifies the
> following line, by splitting the definition of the complicated
> function pointer type from its use.

OK, I'll split it out.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.

Richard


Fixes a “left operand of comma has no effect” warning that some were
seeing.  Also fixes a spurious ellipsis that Jonathan Wakely pointed
out.

2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* coretypes.h (first_type): New alias template.
	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.
	Remove spurious “...” and split the function type out into a typedef.
---
 gcc/coretypes.h | 4 ++++
 gcc/recog.h     | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index cda22697cc3..01ec2e23ce2 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -359,6 +359,10 @@ struct kv_pair
   const ValueType value;	/* the value of the name */
 };
 
+/* Alias of the first type, ignoring the second.  */
+template<typename T1, typename T2>
+using first_type = T1;
+
 #else
 
 struct _dont_use_rtx_here_;
diff --git a/gcc/recog.h b/gcc/recog.h
index 0a71a02c4a9..d674d384723 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -295,9 +295,10 @@ struct insn_gen_fn
   typedef void (*stored_funcptr) (void);
 
   template<typename ...Ts>
-  rtx_insn *operator() (Ts... args...) const
+  rtx_insn *operator() (Ts... args) const
   {
-    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);
+    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);
+    return ((funcptr) func) (args...);
   }
 
   // This is for compatibility of code that invokes functions like