From patchwork Wed Sep 9 10:10:05 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 8604 Received: (qmail 109684 invoked by alias); 9 Sep 2015 10:10:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 108804 invoked by uid 89); 9 Sep 2015 10:10:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 09 Sep 2015 10:10:08 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 79E772FD9A5; Wed, 9 Sep 2015 10:10:07 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t89AA5e7019782; Wed, 9 Sep 2015 06:10:06 -0400 Message-ID: <55F0057D.9040409@redhat.com> Date: Wed, 09 Sep 2015 11:10:05 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Simon Marchi , gdb-patches@sourceware.org, Joel Brobecker Subject: Re: [PATCH] Constify variables in ada-lang.c References: <1440777092-18775-1-git-send-email-simon.marchi@ericsson.com> In-Reply-To: <1440777092-18775-1-git-send-email-simon.marchi@ericsson.com> On 08/28/2015 04:51 PM, Simon Marchi wrote: > I found this const/not const mixup found by building in C++ mode. > > I would push this as obvious, but I am not sure I understand what > happens in scan_discrim_bound, so I'd like it to be reviewed. > > @@ -11446,11 +11445,12 @@ scan_discrim_bound (char *str, int k, struct value *dval, LONGEST * px, > GROW_VECT (bound_buffer, bound_buffer_len, pend - (str + k) + 1); > bound = bound_buffer; > strncpy (bound_buffer, str + k, pend - (str + k)); > - bound[pend - (str + k)] = '\0'; > + bound_buffer[pend - (str + k)] = '\0'; > k = pend - str; > } That seems obvious to me -- bound and bound_buffer point to the same at this point. I think you should go ahead and push the patch in. Maybe as a follow up patch, if we moved the "bound = bound_buffer;" line further below, the code would be a tiny bit more obvious, as both branches of the if will have the same pattern in their last two lines. I'd also assign "str + k" to a temporary; all that pointer arithmetic makes things not as clear as they could be IMO. Something like this (rebased on top of the constification): Thanks, Pedro Alves --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -11431,22 +11431,26 @@ scan_discrim_bound (char *str, int k, struct value *dval, LONGEST * px, char *bound; char *pend; struct value *bound_val; + char *pstart; if (dval == NULL || str == NULL || str[k] == '\0') return 0; - pend = strstr (str + k, "__"); + pstart = str + k; + pend = strstr (pstart, "__"); if (pend == NULL) { - bound = str + k; + bound = pstart; k += strlen (bound); } else { - GROW_VECT (bound_buffer, bound_buffer_len, pend - (str + k) + 1); + /* Strip __ and beyond. */ + GROW_VECT (bound_buffer, bound_buffer_len, pend - pstart + 1); + strncpy (bound_buffer, pstart, pend - pstart); + bound_buffer[pend - pstart] = '\0'; + bound = bound_buffer; - strncpy (bound_buffer, str + k, pend - (str + k)); - bound[pend - (str + k)] = '\0'; k = pend - str; }