Re: [PATCH 15/55] rs6000: Build and store function type identifiers

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Thu, Jun 17, 2021 at 10:18:59AM -0500, Bill Schmidt wrote:
> 	* config/rs6000/rs6000-gen-builtins.c (complete_vector_type): New
> 	function.
> 	(complete_base_type): Likewise.
> 	(construct_fntype_id): Likewise.
> 	(parse_bif_entry): Call contruct_fntype_id.
> 	(parse_ovld_entry): Likewise.

> +/* Convert a vector type into a mode string.  */
> +static void
> +complete_vector_type (typeinfo *typeptr, char *buf, int *bufi)
> +{
> +  if (typeptr->isbool)
> +    buf[(*bufi)++] = 'b';
> +  buf[(*bufi)++] = 'v';
> +  if (typeptr->ispixel)
> +    {
> +      memcpy (&buf[*bufi], "p8hi", 4);
> +      *bufi += 4;

      return;
    }

> +  else

... and then you don't need this, and everything after this loses a
level of indentation.  The power of early outs :-)

> +/* Build a function type descriptor identifier from the return type
> +   and argument types described by PROTOPTR, and store it if it does
> +   not already exist.  Return the identifier.  */
> +static char *
> +construct_fntype_id (prototype *protoptr)
> +{
> +  /* Determine the maximum space for a function type descriptor id.
> +     Each type requires at most 9 characters (6 for the mode*, 1 for
> +     the optional 'u' preceding the mode, 1 for the optional 'p'
> +     preceding the mode, and 1 for an underscore following the mode).
> +     We also need 5 characters for the string "ftype" that separates
> +     the return mode from the argument modes.  The last argument doesn't
> +     need a trailing underscore, but we count that as the one trailing
> +     "ftype" instead.  For the special case of zero arguments, we need 9
> +     for the return type and 7 for "ftype_v".  Finally, we need one
> +     character for the terminating null.  Thus for a function with N
> +     arguments, we need at most 9N+15 characters for N>0, otherwise 17.
> +     ----
> +       *Worst case is bv16qi for "vector bool char".  */
> +  int len = protoptr->nargs ? (protoptr->nargs + 1) * 9 + 6 : 17;
> +  char *buf = (char *) malloc (len);

I would completely avoid all this by using asprintf.  But that requires
a little restructuring, so *shrug* (it could use some factoring anyway,
but we don't need this to be perfect ;-) )

> +      assert (!argptr);
> +      }

Formatting here is broken...  Probably that last line has two spaces
to many, but please check.

> +  /* Ignore return value, as duplicates are expected.  */
> +  (void) rbt_insert (&fntype_rbt, buf);

Casting to void does not ignore a return value.  But is rbt_insert
marked up as warn_unused_result anyway?  Simply not using the return
value is fine as well.  If you want to be very explicit, you can write

  if (rbt_insert (&fntype_rbt, buf))
    ; /* Duplicates are fine and expected here.  */

Okay for trunk with or without such improvements.  But do fix that
indentation thing please ;-)  Thanks!


Segher