On 5/21/21 6:43 PM, Segher Boessenkool wrote:
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.
I agree it would be nice if all the gen* tools had line/column
reporting. Maybe we could look at that as a follow-up? I was trying to
keep the patch series as simple as possible, since it's already pretty
large.
+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?
<...my extraneous response and follow-ups deleted...>
Sorry, I wrote this a long time ago and forgot what I was doing here.
Yes, that would be a reasonable thing to do. Even more reasonable would
be to do what I thought I remembered doing...
It would be best to just save these as strings, not integers, since I'm
just going to fprintf them back to a file later anyway. All of these are
strings of at most two digits (with possible leading minus), so just
using isdigit to find the strings is efficient. Using sscanf (or
strtol) and fprintf is just silly. I'll rework to remove that.
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.
ok. Not worth a big argument. ;-)
+ {
+ (*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?
Mostly. I'll have to make sure the line/column reporting is still
sensible, which is the tricky bit. But that's for later...
Thanks again for the review!
Bill
Checking for errors repeatedly is so error-prone :-(
Segher
