Re: [PATCH 06/57] rs6000: Add helper functions for parsing

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

Hi!

On Tue, Apr 27, 2021 at 10:32:41AM -0500, Bill Schmidt via Gcc-patches wrote:
> +/* Used as a sentinel for range constraints on integer fields.  No field can
> +   be 32 bits wide, so this is a safe sentinel value.  */
> +#define MININT INT32_MIN

INT32_MIN is for value of type int32_t.  You use it for "int" though.
There is INT_MIN just for that :-)

> +/* Exit codes for the shell.  */
> +enum exit_codes {
> +  EC_INTERR
> +};

Please define this with some specified value (so append " = 1" or such),
or just write "exit(1);" etc. directly.  As it is, a compiler is free to
do "exit(0);" for this error value!  Not good.

All these exit codes are externally visible, so please just make them
explicit.  There isn't much point in having symbolic names for it
either, because users will see the raw numbers (reported by their
shell), instead.

Most generator programs allow to use fatal() etc., this is a much richer
environment, no need to reinvent stuff?  Include "errors.h" and link
with error.o, and that is all you need AFAIK.

> +static void
> +consume_whitespace ()

Please say (void), empty argument lists have different meanings in
different language versions, and it looks like you just forgot to fill
int the type.  Besides, it is just "what we do" :-)

> +/* Get the next nonblank, noncomment line, returning 0 on EOF, 1 otherwise.  */
> +static int
> +advance_line (FILE *file)
> +{
> +  while (1)
> +    {
> +      /* Read ahead one line and check for EOF.  */
> +      if (!fgets (linebuf, sizeof(linebuf), file))

sizeof is an operator, not a function, and even if it was a function it
would have a space before the opening parenthesis :-)

> +	return 0;

So it also returns 0 on any error?  This may not be a good idea.

> +/* Match an integer and return its value, or MININT on failure.  */
> +static int
> +match_integer ()
> +{
> +  int startpos = pos;
> +  if (linebuf[pos] == '-')
> +    safe_inc_pos ();
> +
> +  int lastpos = pos - 1;
> +  while (isdigit (linebuf[lastpos + 1]))
> +    if (++lastpos >= LINELEN - 1)
> +      {
> +	(*diag) ("line length overrun in match_integer.\n");
> +	exit (EC_INTERR);
> +      }
> +
> +  if (lastpos < pos)
> +    return MININT;
> +
> +  pos = lastpos + 1;
> +  char *buf = (char *) malloc (lastpos - startpos + 2);
> +  memcpy (buf, &linebuf[startpos], lastpos - startpos + 1);
> +  buf[lastpos - startpos + 1] = '\0';
> +
> +  int x;
> +  sscanf (buf, "%d", &x);
> +  return x;
> +}

Can't you just use strtol?

> +static const char *
> +match_to_right_bracket ()

This needs a function comment.

> +{
> +  int lastpos = pos - 1;
> +  while (linebuf[lastpos + 1] != ']')
> +    if (++lastpos >= LINELEN - 1)

Please don't use side effects in "if" conditions.

> +      {
> +	(*diag) ("line length overrun.\n");
> +	exit (EC_INTERR);
> +      }

I don't think you shoulod check for line length overrun in any of these
functions, btw?  Just check where you read them in, and that is plenty?

> +  if (lastpos < pos)
> +    return 0;
> +
> +  char *buf = (char *) malloc (lastpos - pos + 2);
> +  memcpy (buf, &linebuf[pos], lastpos - pos + 1);
> +  buf[lastpos - pos + 1] = '\0';
> +
> +  pos = lastpos + 1;
> +  return buf;
> +}

Are there no utility routines you can use?  It would be useful to have
something that all gen* can use (something less bare than what there is
now...)


Segher