From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id B21C2385803B for ; Fri, 21 May 2021 18:52:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B21C2385803B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 14LIpQHf024893; Fri, 21 May 2021 13:51:26 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 14LIpP7E024892; Fri, 21 May 2021 13:51:25 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 21 May 2021 13:51:25 -0500 From: Segher Boessenkool To: Bill Schmidt Cc: gcc-patches@gcc.gnu.org, jakub@redhat.com, jlaw@tachyum.com, dje.gcc@gmail.com Subject: Re: [PATCH 06/57] rs6000: Add helper functions for parsing Message-ID: <20210521185125.GB10366@gate.crashing.org> References: <339c02294ee06570958bc71c772f33736a103fa9.1619537141.git.wschmidt@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <339c02294ee06570958bc71c772f33736a103fa9.1619537141.git.wschmidt@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 May 2021 18:52:29 -0000 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