Re: [PATCH 18/18] rs6000: Add escape-newline support for builtins files

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

Hi!

On Wed, Sep 01, 2021 at 11:13:54AM -0500, Bill Schmidt wrote:
> +/* Escape-newline support.  For readability, we prefer to allow developers
> +   to use escape-newline to continue long lines to the next one.  We
> +   maintain a buffer of "original" lines here, which are concatenated into
> +   linebuf, above, and which can be used to convert the virtual line
> +   position "line / pos" into actual line and position information.  */
> +#define MAXLINES 4

Make this bigger already?  Or, want to bet if we will need to increase
it for GCC 12 already?  Because for GCC 13 we almost certainly will :-)

> +/* From a possibly extended line with a virtual position, calculate
> +   the current line and character position.  */
> +static void
> +real_line_pos (int diagpos, int *real_line, int *real_pos)
> +{
> +  *real_line = line - lastline;
> +  *real_pos = diagpos;
> +
> +  for (int i = 0; i < MAXLINES && *real_pos > (int) strlen (lines[i]); i++)
> +    {
> +      (*real_line)++;
> +      *real_pos -= strlen (lines[i]) - 2;
> +    }
> +
> +  /* Convert from zero-base to one-base for printing.  */
> +  (*real_pos)++;
> +}

The cast is nasty, and you reuse that expression (which includes an
expensive call, which GCC might not realise it can CSE) anyway.  You
can rewrite this like

  for (int i = 0; i < MAXLINES; i++)
    {
      int len = strlen (lines[i]);
      if (*real_pos > len)
	break;

      (*real_line)++;
      *real_pos -= len - 2;
    }

fixing both these issues.

> +/* Produce a fatal error message.  */
> +static void
> +fatal (const char *msg)
> +{
> +  fprintf (stderr, "FATAL: %s\n", msg);
> +  abort ();
> +}

No vfprint?  Aww :-)  You didn't yet see any use for formatted fatal
errors I guess.

> +	      if (lastline == MAXLINES - 1)
> +		fatal ("number of supported overflow lines exceeded");
> +	      lastline++;

If you test after the increment you don't need the - 1 ;-)

> +	      line++;
> +	      if (!fgets (lines[lastline], LINELEN, file))
> +		fatal ("unexpected end of file");
> +	      strcpy (&linebuf[len - 2], lines[lastline]);

So, can this overflow linebuf?  I didn't see a test for that.

> +  /* Allocate some buffers.  */
> +  for (int i = 0; i < MAXLINES; i++)
> +    lines[i] = (char *) malloc (LINELEN);

C++ forces such unsightly casts, sigh.  Well there are worse things.
Some certain operator comes to mind.


Thanks for this cleanup!  In the new builtin definitions lines are much
shorter than before, but a few got really long anyway :-)

Okay for trunk.  Thanks!


Segher