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

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

Hi!

On Fri, May 21, 2021 at 03:56:09PM -0500, Bill Schmidt wrote:
> On 5/21/21 1:51 PM, Segher Boessenkool wrote:
> >>+/* 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.
> 
> I'll dump the exit codes, which were an early idea that didn't prove 
> useful in the end.  exit (1) will suffice...
> 
> However, I'd like to keep my approach to diagnostics.  I wrote it this 
> way so that I would have a centralized place to produce the file, line, 
> and column information, which was really helpful while debugging these 
> large input files.  Putting wrappers around the errors.c functions seems 
> at least as messy.

Yes, wrappers is a no-go.  But you could just have added the features
you need to the generic code?  Was there a technical reason not to do
that?  It sounds useful in many places, not just here.

> >>+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?
> I could, but (a) it's more overhead because of tracking the base, and 

How so?  If you give base 0 it handles all of decimal, hexadecimal, and
octal (with the usual 0x and 0 prefixes).

> (b) I then have to calculate lastpos afterwards.  /shrug

It can stores it in where its second arg points to!

===
  char *endp;
  const char *str = where_the_number_starts_or_some_whitespace_before_it;

  errno = 0;
  long n = strtol (str, &endp, 0);
  if (!*str || errno)
    whoops (...);
===

... and now endp points to the first char that is not part of the
number.  It's one of the cases where errno is *helpful* :-)

> >>+  int lastpos = pos - 1;
> >>+  while (linebuf[lastpos + 1] != ']')
> >>+    if (++lastpos >= LINELEN - 1)
> >Please don't use side effects in "if" conditions.
> 
> Really?  Is it actually better to write
> 
>   while (linebuf[lastpos + a] != ']')
>     {
>       ++lastpos;
>       if (lastpos >= LINELEN - 1)
>         ...
>     }

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

      ...
    }

> Frankly I don't see it...and I don't see anything in the GNU or GCC 
> coding conventions about this.  I'd rather keep what I have.

The most used side effect in conditionals is assignment.  This saves a
few keystrokes, and maybe a whole line in the code, but it makes the
code a lot harder to comprehend.

In your code it does not matter so very much, since you exit if there
is an error anyway, but it makes it a lot harder to verify what the code
does in all cases, and to check that that is what is wanted.

> >>+      {
> >>+	(*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?
> 
> Yes -- I think if I check in advance_line for a line that doesn't end in 
> \n, that will make a lot of those things superfluous.
> 
> I've been a little reluctant to do that, since eventually I want to 
> support escape-newline processing to avoid long input lines, but I can 
> still work around that when I get to it.

Aha, that explains.  Yeah you should be able to check the length again
when concatenating two lines, and that is all you need?

Checking for errors repeatedly is so error-prone :-(


Segher