From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3881 invoked by alias); 4 Aug 2010 21:56:52 -0000 Received: (qmail 3872 invoked by uid 22791); 4 Aug 2010 21:56:51 -0000 X-SWARE-Spam-Status: No, hits=0.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KAM_STOCKTIP,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (74.125.121.35) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 04 Aug 2010 21:56:46 +0000 Received: from kpbe14.cbf.corp.google.com (kpbe14.cbf.corp.google.com [172.25.105.78]) by smtp-out.google.com with ESMTP id o74Luh14013334 for ; Wed, 4 Aug 2010 14:56:43 -0700 Received: from vws18 (vws18.prod.google.com [10.241.21.146]) by kpbe14.cbf.corp.google.com with ESMTP id o74Lu5AT001306 for ; Wed, 4 Aug 2010 14:56:42 -0700 Received: by vws18 with SMTP id 18so5199921vws.15 for ; Wed, 04 Aug 2010 14:56:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.59.202 with SMTP id m10mr6579667vch.199.1280959001654; Wed, 04 Aug 2010 14:56:41 -0700 (PDT) Received: by 10.220.176.2 with HTTP; Wed, 4 Aug 2010 14:56:41 -0700 (PDT) In-Reply-To: <6308127460174723176@unknownmsgid> References: <4C582E25.1030505@redhat.com> <6308127460174723176@unknownmsgid> Date: Wed, 04 Aug 2010 21:56:00 -0000 Message-ID: Subject: Re: [patch] create and use symbol_set_language From: Doug Evans To: Pierre Muller Cc: sami wagiaalla , gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true X-IsSubscribed: yes 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 X-SW-Source: 2010-08/txt/msg00021.txt.bz2 On Wed, Aug 4, 2010 at 12:27 AM, Pierre Muller wrote: > Hi Sami, > > =A0I just looked at your patch, which seems > quite straightforward. > > =A0Nevertheless, it seems that > it contains a change that is not commented: > > @@ -393,13 +393,11 @@ symbol_get_demangled_name (const struct > general_symbol_info *gsymbol) > =A0/* Initialize the language dependent portion of a symbol > =A0 =A0depending upon the language for the symbol. */ > =A0void > -symbol_init_language_specific (struct general_symbol_info *gsymbol, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0enum languag= e language) > +symbol_set_language (struct general_symbol_info *gsymbol, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 enum language language) > =A0{ > - > =A0 gsymbol->language =3D language; > - =A0if (gsymbol->language =3D=3D language_cplus > - =A0 =A0 =A0|| gsymbol->language =3D=3D language_d > + =A0if (gsymbol->language =3D=3D language_d > =A0 =A0 =A0 || gsymbol->language =3D=3D language_java > =A0 =A0 =A0 || gsymbol->language =3D=3D language_objc > =A0 =A0 =A0 || gsymbol->language =3D=3D language_fortran) > > =A0The removal of the 'gsymbol->language =3D=3D language_cplus' > condition seems to be outside of the scope of the patch you > describe, which seems otherwise quite straightforward. > > =A0Could you please comment on the reason of that specific change? > Is it really part of that patch or shouldn't it be submitted > separately? I must confess that I didn't even try to > look at the source code after the 'if', but just reacted quickly > on something that seem 'off topic' as compared to > your patch description. Yeah. This part of the patch is correct, it just needs a ChangeLog entry. [One could submit it separately, but I don't mind it being included here.] What's happening here is that language_cplus is being checked for twice. Here's the current definition of the function. void symbol_init_language_specific (struct general_symbol_info *gsymbol, enum language language) { gsymbol->language =3D language; if (gsymbol->language =3D=3D language_cplus || gsymbol->language =3D=3D language_d || gsymbol->language =3D=3D language_java || gsymbol->language =3D=3D language_objc || gsymbol->language =3D=3D language_fortran) { symbol_set_demangled_name (gsymbol, NULL, NULL); } else if (gsymbol->language =3D=3D language_cplus) gsymbol->language_specific.cplus_specific =3D NULL; else { memset (&gsymbol->language_specific, 0, sizeof (gsymbol->language_specific)); } } The patch is generally ok, but a bit more is required: - the additional ChangeLog entry to mention fixing test of language_cplus - after applying the patch, grepping for SYMBOL_INIT still has a few hits that need to be addressed: gdb$ grep SYMBOL_INIT *.[ch] buildsym.c:#include "demangle.h" /* Needed by SYMBOL_INIT_DEMANGLED_NAME. */ symtab.h: the SYMBOL_INIT_LANGUAGE_SPECIFIC, SYMBOL_DEMANGLED_NAME, etc. - one potential formatting nit: diff --git a/gdb/stabsread.c b/gdb/stabsread.c index b62156c..ea9d1e0 100644 --- a/gdb/stabsread.c +++ b/gdb/stabsread.c @@ -704,7 +704,7 @@ define_symbol (CORE_ADDR valu, char *string, int desc, int type, else { normal: - SYMBOL_LANGUAGE (sym) =3D current_subfile->language; + SYMBOL_SET_LANGUAGE (sym, current_subfile->language); if (SYMBOL_LANGUAGE (sym) =3D=3D language_cplus) { char *name =3D alloca (p - string + 1); I'm happy with the patch with those changes.