On 5/21/21 1:51 PM, Segher Boessenkool wrote:
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 :-)
OK.
+/* 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.
+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" :-)
OK.
+/* 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 :-)
OK.
+ return 0;
So it also returns 0 on any error? This may not be a good idea.
match_identifier returns 0 when the string doesn't match a legal
identifier, as described in the function note. Otherwise the string is
copied into a dynamic buffer and its address is returned. Not clear to
me what you're objecting to here.
+/* 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?
I could, but (a) it's more overhead because of tracking the base, and
(b) I then have to calculate lastpos afterwards. /shrug
+static const char *
+match_to_right_bracket ()
This needs a function comment.
OK.
+{
+ 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)
...
}
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.
+ {
+ (*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.
+ 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...)
I didn't find anything great as I was poking around, hence I wrote my
own low level utilities. It goes back to my desire to track line/pos
information for debug.
Thanks for the review!
Bill
Segher
