From: Joel Brobecker <brobecker@adacore.com>
To: Vladimir Prus <vladimir@codesourcery.com>
Cc: gdb-patches@sources.redhat.com
Subject: Re: [mi] [ada] varobjs for registers 2
Date: Thu, 21 Dec 2006 07:20:00 -0000 [thread overview]
Message-ID: <20061221072102.GF3640@adacore.com> (raw)
In-Reply-To: <200612202137.29038.vladimir@codesourcery.com>
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
next prev parent reply other threads:[~2006-12-21 7:20 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-12-20 18:38 Vladimir Prus
2006-12-20 19:44 ` Eli Zaretskii
2007-01-05 9:01 ` Vladimir Prus
2007-01-05 9:29 ` Eli Zaretskii
2006-12-21 7:20 ` Joel Brobecker [this message]
2006-12-30 20:30 ` Daniel Jacobowitz
2007-01-05 9:06 ` Vladimir Prus
2007-01-05 10:24 ` Nick Roberts
2007-01-05 10:47 ` Vladimir Prus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20061221072102.GF3640@adacore.com \
--to=brobecker@adacore.com \
--cc=gdb-patches@sources.redhat.com \
--cc=vladimir@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox