This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [mi] [ada] varobjs for registers 2


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]