From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32204 invoked by alias); 21 Dec 2006 07:20:30 -0000 Received: (qmail 32196 invoked by uid 22791); 21 Dec 2006 07:20:29 -0000 X-Spam-Check-By: sourceware.org Received: from nile.gnat.com (HELO nile.gnat.com) (205.232.38.5) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 21 Dec 2006 07:20:19 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-nile.gnat.com (Postfix) with ESMTP id 15F0548CE0A; Thu, 21 Dec 2006 02:20:17 -0500 (EST) Received: from nile.gnat.com ([127.0.0.1]) by localhost (nile.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 09331-01-6; Thu, 21 Dec 2006 02:20:16 -0500 (EST) Received: from takamaka.act-europe.fr (AStDenis-105-1-71-21.w80-8.abo.wanadoo.fr [80.8.210.21]) by nile.gnat.com (Postfix) with ESMTP id 2C2EB48CBB7; Thu, 21 Dec 2006 02:20:16 -0500 (EST) Received: by takamaka.act-europe.fr (Postfix, from userid 1000) id 4A05934C099; Thu, 21 Dec 2006 11:21:02 +0400 (RET) Date: Thu, 21 Dec 2006 07:20:00 -0000 From: Joel Brobecker To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: [mi] [ada] varobjs for registers 2 Message-ID: <20061221072102.GF3640@adacore.com> References: <200612202137.29038.vladimir@codesourcery.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200612202137.29038.vladimir@codesourcery.com> User-Agent: Mutt/1.4.2.2i 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: 2006-12/txt/msg00280.txt.bz2 Volodya, (or should I call you Vladimir?) > Along the way, I've promoted a private function in ada-lang.c to language.c, > so I'd appreciate if Ada maintainers take a quick look. I had a look, and the change looks fine. I also tested it against our testsuite, and it showed no regression, which should bring us additional confidence. Please be aware that I am not a maintainer, and thus this review is only informal, and thus is not an approval. While scanning the patch, I did notices a couple of minor things: > @@ -4226,10 +4220,10 @@ lookup_symbol_in_language (const char *n > domain_enum domain, enum language lang, > int *is_a_field_of_this, struct symtab **symtab) > { > + enum language prev_lang = set_language (lang); > struct cleanup *old_chain > - = make_cleanup (restore_language, (void *) current_language->la_language); > + = make_cleanup (restore_language, &prev_lang); The two lines can now be joined (ie the make_cleanup call can be joined with the previous line). > +static void restore_language_mode (void *p) > +{ > + enum language_mode *mode = p; > + language_mode = *mode; > +} The formatting is wrong. You need a line break after "static void" so that "restore_language_mode" is at the begining of the next line. Also, perhaps this function should be moved to language.c as well? > +/* Cast 'lang' to 'enum language *', dererence it, and > + set the current language to the value. > + > + This function is intended to be used as cleanup function. */ > +extern void restore_language (void *lang); I believe the comment describing the function should be placed besides the function implementation, in language.c. This is the habit we have at least in the GNU projects I've been involved in. In Ada, we much prefer to place the documentation besides the declaration, as you did, but C allows you to put as many declarations of the same entity as you want, and wherever you want. So this conventions allows us to know exactly where the documentation is... -- Joel