This is the mail archive of the mailing list for the binutils 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: [patch] General info in AS listings

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  <>
+	* 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



Please email the following information to, 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?]


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