Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Joel Brobecker <brobecker@adacore.com>
To: Pierre Muller <muller@ics.u-strasbg.fr>
Cc: gdb-patches@sourceware.org, gpc@gnu.de
Subject: Re: [RFC] Handle GPC specific name for main function
Date: Wed, 26 Sep 2007 17:58:00 -0000	[thread overview]
Message-ID: <20070926175805.GA6208@adacore.com> (raw)
In-Reply-To: <003901c8004b$2f9a55d0$8ecf0170$@u-strasbg.fr>

Hello Pierre,

> 2007-09-26  Pierre Muller  <muller@ics.u-strasbg.fr>
> 	* p-lang.h (pascal_main_name): New function.
> 	p-lang.c (GPC_MAIN_PROGRAM_NAME_1,
> 	GPC_MAIN_PROGRAM_NAME_2): New char array constants
> 	corresponding to the two minimal symbols used 
> 	by GPC compiler.
> 	(pascal_main_name): Try to find minimal symbol
> 	corresponding to the entry of GPC compiled programs.
> 	symtab.c (find_main_name): Try to find
> 	pascal specific main name by calling pascal_main_name.

This is mostly OK, except for the tiny comment formatting pointed out
by Eli. I have a few suggestions:

> +/* The name of the symbol used by GPC compiler.  */

I find this is too vague. I suggest something like:

  /* The name of the symbol that GPC uses as the name of the main
     subprogram.  There are several possibilities depending on
     the version of GPC.  */

I might even go one step further and put a comment for each entry.
Something like:

  /* The name of the symbol that GPC uses as the name of the main
     subprogram (since version ...).  */

And then, for the other one say:

  /* Older versions of GPC (version ... and older) were using
     a different name for the main subprogram.  */

> +static const char GPC_MAIN_PROGRAM_NAME_1[]
> +  = "pascal_main_program";
> +static const char GPC_MAIN_PROGRAM_NAME_2[]
> +  = "_p__M0_main_program";

Also, I think you might want to order them the opposite way.
The net effect is that you'll end up trying the newer symbol
first, and then fallback to the older symbol. When people
start using the newer compiler, GDB will need to do only one
symbol lookup to find a match.

> +  /* For GPC, main procedure s a special name.
> +     .  */

I think making the comment above a bit more expressive makes
this comment superfluous.

> @@ -40,6 +40,7 @@
>  #include "filenames.h"         /* for FILENAME_CMP */
>  #include "objc-lang.h"
>  #include "ada-lang.h"
> +#include "p-lang.h"
> 
>  #include "hashtab.h"
> 

You also need to update Makefile.in, since you added a dependency.

With these adjustments in, your patch is pre-approved (but please
remember to give it a round of testing, just in cases).

Thank you,
-- 
Joel


  parent reply	other threads:[~2007-09-26 17:58 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-09-26 14:40 Pierre Muller
2007-09-26 15:03 ` Eli Zaretskii
2007-09-26 15:32   ` Pierre Muller
2007-09-26 17:48     ` Eli Zaretskii
2007-09-26 17:58 ` Joel Brobecker [this message]
2007-09-26 19:39   ` Pierre Muller
2007-09-26 19:50     ` Joel Brobecker
2007-09-26 22:06       ` Pierre Muller
2007-09-26 22:39         ` [RFC-2] " Pierre Muller
2007-09-27  6:03           ` Joel Brobecker
2007-09-27  7:29             ` [RFC-3] " Pierre Muller
     [not found]               ` <46FB5E2C.6080606@microbizz.nl>
     [not found]                 ` <46FB5F76.9050501@microbizz.nl>
2007-09-27  7:57                   ` Pierre Muller
2007-09-27 12:11                     ` Daniel Jacobowitz
2007-09-27 12:35                       ` Pierre Muller
2007-09-27 12:40                         ` 'Daniel Jacobowitz'
2007-09-27 16:20                           ` Joel Brobecker
2007-09-27 16:32                             ` Joel Brobecker
2007-09-27 21:36                               ` Pierre Muller
2007-09-28 18:31                                 ` Joel Brobecker
2007-09-27  8:02                 ` Pierre Muller
2007-09-27 13:01                   ` Waldek Hebisch
2007-09-27  7:20           ` [RFC-2] " Eli Zaretskii
2007-09-27  1:58 ` [RFC] " Waldek Hebisch
2007-09-27  5:52   ` Joel Brobecker
2007-09-27 17:17     ` Thiago Jung Bauermann
2007-09-27 19:50       ` Jim Blandy

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=20070926175805.GA6208@adacore.com \
    --to=brobecker@adacore.com \
    --cc=gdb-patches@sourceware.org \
    --cc=gpc@gnu.de \
    --cc=muller@ics.u-strasbg.fr \
    /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