From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 33DC93858024 for ; Tue, 1 Jun 2021 15:50:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 33DC93858024 Received: from pps.filterd (m0187473.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 151FmhNu144577; Tue, 1 Jun 2021 11:50:34 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 38wqv4g1tx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 01 Jun 2021 11:50:33 -0400 Received: from m0187473.ppops.net (m0187473.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 151Fo8Id149584; Tue, 1 Jun 2021 11:50:33 -0400 Received: from ppma04dal.us.ibm.com (7a.29.35a9.ip4.static.sl-reverse.com [169.53.41.122]) by mx0a-001b2d01.pphosted.com with ESMTP id 38wqv4g1tn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 01 Jun 2021 11:50:33 -0400 Received: from pps.filterd (ppma04dal.us.ibm.com [127.0.0.1]) by ppma04dal.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 151Fn5cA022471; Tue, 1 Jun 2021 15:50:32 GMT Received: from b03cxnp08026.gho.boulder.ibm.com (b03cxnp08026.gho.boulder.ibm.com [9.17.130.18]) by ppma04dal.us.ibm.com with ESMTP id 38ud89phbu-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 01 Jun 2021 15:50:32 +0000 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 151FoVuC15794566 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 1 Jun 2021 15:50:31 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 75D327805E; Tue, 1 Jun 2021 15:50:31 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 460D57805F; Tue, 1 Jun 2021 15:50:31 +0000 (GMT) Received: from Bills-MacBook-Pro.local (unknown [9.163.5.209]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Tue, 1 Jun 2021 15:50:31 +0000 (GMT) Reply-To: wschmidt@linux.ibm.com Subject: Re: [PATCH 06/57] rs6000: Add helper functions for parsing To: Segher Boessenkool Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com References: <339c02294ee06570958bc71c772f33736a103fa9.1619537141.git.wschmidt@linux.ibm.com> <20210521185125.GB10366@gate.crashing.org> <784f02c0-f452-c7db-c9a9-94f04cbf1bd2@linux.ibm.com> <20210521234330.GG10366@gate.crashing.org> From: Bill Schmidt Message-ID: <74532260-2018-bbf9-bf70-e03424efc1c1@linux.ibm.com> Date: Tue, 1 Jun 2021 10:50:30 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 MIME-Version: 1.0 In-Reply-To: <20210521234330.GG10366@gate.crashing.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-TM-AS-GCONF: 00 X-Proofpoint-GUID: W31pxxQPIW0Q_O3tndpFL2e43m8ZV0D5 X-Proofpoint-ORIG-GUID: t388EOedkIfhtCGHAY7wwnL7Y57cogUI X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391, 18.0.761 definitions=2021-06-01_08:2021-06-01, 2021-06-01 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 impostorscore=0 lowpriorityscore=0 mlxlogscore=999 malwarescore=0 mlxscore=0 suspectscore=0 spamscore=0 priorityscore=1501 clxscore=1015 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2106010102 X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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: Tue, 01 Jun 2021 15:50:36 -0000 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