Re: [patch] General info in AS listings

[Nick Clifton <nickc at redhat dot com>]

Hi Santiago,

> Hi, this simple patch adds a new 'g' sub-option to the '-a' gas switch
> to print general information in assembly listings like gas version,
> options passed, or time stamp.

Thanks very much for submitting this patch.  The first thing that I must 
point out is that, as you anticipated, in order for us to be able to 
accept patches from you, you need to complete a copyright assignment 
with the FSF.  This must be done by you if the work is all your own, or 
by your employer if the work was done for them (or using their 
machines!).  I have attached the necessary form for you to fill out and 
send off in order to start this process rolling.

The next thing to consider is whether the feature is actually necessary 
?  For example it seems to me that all of the information generated by 
your new option could also be obtained from the command line or via a 
wrapper script.  So why add the option to the assembler ?  The point 
here being that it is a bad idea to add unnecessary options - feature 
bloat - because it just makes room for more bugs.


> The patch includes the Changefile and texi changes.

Which is wonderful to see. :-)  One minor point is that when you are 
adding a new feature, rather than just fixing a bug, you should also 
include a patch to update the gas/NEWS file (or ld/NEWS file, etc).

Plus, the other part that is often forgotten when adding a new feature 
is to add one or more tests to the testsuite to check that the feature 
works and that it continues to work in the future.


I also have a few comments on the patch itself:

> +2008-03-30  Santiago Urueña  <suruena@gmail.com>
> +
> +	* listing.c (listing_general_info): Additional listing flag
> +	to show general information in listings like passed options
> +	and gas version.
> +

It is customary to include the ChangeLog entry for a patch just as plain 
text, rather than as a context diff.  This is because the ChangeLogs 
change so rapidly that a context diff almost never applies cleanly.

> @@ -370,10 +371,6 @@
>  static void
>  parse_args (int * pargc, char *** pargv)
>  {
> -  int old_argc;
> -  int new_argc;
> -  char ** old_argv;
> -  char ** new_argv;
>    /* Starting the short option string with '-' is for programs that
>       expect options and other ARGV-elements in any order and that care about
>       the ordering of the two.  We describe each non-option ARGV-element

This looks wrong to me.  Why make new_argc and new_argv into global 
variables when they are only ever used inside the parse_args() function ?

> +/* Print options passed to as. */

Comments should end in a period followed by two spaces, not just one.

Otherwise it seems fine to me.


Cheers
  Nick


---------------------------------------------------------------------------

request-assign.future:

Please email the following information to fsf-records@gnu.org, and we
will send you the assignment form for your past and future changes.
Please use your full name as the subject line of the message.


[What is the name of the program or package you're contributing to?]


[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]


[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]


[For the copyright registration, what country are you a citizen of?]


[What year were you born?]


[Please write your email address here.]


[Please write your snail address here.]





[Which files have you changed so far, and which new files have you written
so far?]





---------------------------------------------------------------------------