From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22482 invoked by alias); 17 Apr 2013 20:33:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22457 invoked by uid 89); 17 Apr 2013 20:33:40 -0000 X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 17 Apr 2013 20:33:39 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3HKXalw018242 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 17 Apr 2013 16:33:36 -0400 Received: from host2.jankratochvil.net (ovpn-116-84.ams2.redhat.com [10.36.116.84]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3HKXUJK006715 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 17 Apr 2013 16:33:33 -0400 Date: Thu, 18 Apr 2013 10:08:00 -0000 From: Jan Kratochvil To: Aleksandar Ristovski Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 6/8] gdbserver build-id attribute generator Message-ID: <20130417203330.GE2090@host2.jankratochvil.net> References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1365521265-28870-7-git-send-email-ARistovski@qnx.com> <20130414141658.GD23227@host2.jankratochvil.net> <516D6AE7.8030700@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <516D6AE7.8030700@qnx.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-04/txt/msg00551.txt.bz2 On Tue, 16 Apr 2013 17:14:47 +0200, Aleksandar Ristovski wrote: > On 13-04-14 10:16 AM, Jan Kratochvil wrote: > >On Tue, 09 Apr 2013 17:27:43 +0200, Aleksandar Ristovski wrote: > >>+ break; > >>+ } > >>+ > >>+ nhdr = (void *) pt_note; > >>+ while ((void *) nhdr < (void *) pt_end) > > > >When it is all const there should be (const void *). But in fact it is easier > >to use const gdb_byte * when pt_end is already such type: > > while ((const gdb_byte *) nhdr < pt_end) > > gdb_byte * is o.k., but there is no really a need for const - when > casting is for the purpose of pointer comparison, IMO const only > clutters the reading. As stated also elsewhere in this review one of the review purposes is to keep the GDB (and GNU) coding unified (when not explicitly defined by GNU Coding Standards). GDB was not formerly using const at all, it is moving now towards to const-correct typing. AFAIK const is never used for values in GDB, only for targets. Too many const attributes are a problem for GNU Coding Standards compliant code as the narrow 80 columns formatting then needs multi-line statements which are difficult to read. But when I agreed even with const values when you are used to it please do not break the const-correctness afterwards. I do not see what is a purpose of all the const attributes when in the end there are free de-const-ing casts. > >>+ { > >>+ const size_t namesz > >>+ = ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_namesz)); > >>+ const size_t descsz > >>+ = ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_descsz)); > >>+ const size_t note_sz = ELFXX_SIZEOF (*nhdr) + namesz + descsz; > >>+ > >>+ if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0 > > > >It should be (const gdb_byte *) because nhdr is already const *. > > I like using const, as you may have noticed, unless it only > clutters, like is the case when casting is used for the purpose of > pointer comparison like here. It really does not matter what you like, it matters what is the current GDB coding practice. And when a pointer is const type * I find a bug to cast it to non-const type *. What if the caller really pointed it to a read-only segment. non-const type * is at least confusing in such case. > >>+ break; > >>+ } > >>+ if (ELFXX_FLD (*nhdr, n_type) == NT_GNU_BUILD_ID > >>+ && ELFXX_FLD (*nhdr, n_namesz) == 4) > >>+ { > >>+ const char gnu[4] = "GNU\0"; > >>+ const char *const pname > >>+ = (char *) nhdr + ELFXX_SIZEOF (*nhdr); > > > >It should be (const char *) because nhdr is already const *. > > When assigning to const, then const in the cast for pointer > arithmetic does not matter and IMO only clutters. const * cast to non-const * cast is a bug. When there is even no agreement on how to use const it is better to keep the GDB standards and not to use too many const keywords. > >>+ } > >> { > >> /* Expand to guarantee sufficient storage. */ > >>- uintptr_t document_len = p - document; > >>+ const ptrdiff_t document_len = p - document; > >> > >>- document = xrealloc (document, 2 * allocated); > >> allocated *= 2; > >>+ document = xrealloc (document, allocated); > >> p = document + document_len; > >> } > > > >This "code cleanup" change is unrelated to this patch. But it is IMO not > >worth checking in separately, it does not improve it anyhow IMO. > > It uses correct type for pointer difference and does not repeat > arithmetic. I removed it, it will probably reduce clash with Gary's > patch. You are right ptrdiff_t is more correct there but this 64-bit correctness is unreal to be ever violated. GDB has more serious 64-bit sizes violations... Thanks, Jan