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: gdb-patch mailing list


Joel,

Ok, I'll have a look at it... I don't have any other thing to do anyway. In the mean time you could perhaps try to find out why my email doesn't appear on the mailing list anymore.

Michael.

Joel Brobecker wrote:
Michael,

The gdb-patches mailing list should work. Can you try using it for
the next iteration?

Again, please state the intent of the patch. What are you fixing?
I can often guess from the patch itself, but sometimes I get it wrong.
A good way of presenting your change, for instance, is to show the debugger
output before and after your patch,

I am realizing that perhaps you don't know how to read the contents
of patch files? For instance:

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 753fbb0..bc71679 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog

This bit of the patch says that you are modifying the ChangeLog. I mentioned in a previous message that it's simpler for you to send the entry as inlined text inside the body of your email, rather than as a patch, but I don't mind. However, looking further into the patch, it says that you are proposing the following modifications to this file:


- Add new tracepoint action teval.
- * tracepoint.c (teval_pseudocommand): New function.
- (validate_actionline): Add teval action case.
- (encode_actions): Ditto.
- (_initialize_tracepoint): Define teval pseudocommand.
- * NEWS: Mention teval.
[...]

The leading minus '-' signs mean that you propose to remove this entry
(and many other entries). I don't think you really meant that.

I couldn't locate any change that seemed to add an entry, but maybe
I missed it, since the diff was so large and mostly irrelevant.

-/* Print the status word STATUS. */
-
-static void
-print_i387_status_word (unsigned int status, struct ui_file *file)
+/* print the status word */
+static void print_i387_status_word (unsigned int status, struct ui_file *file)

This part of the patch says that: (1) You are replacing the following comment: /* Print the status word STATUS. */ with: /* print the status word */ (2) You are removing the newline between "static void" and the function name "print_i387_status_word"

I don't think that (1) was really intended, was it? Your version
brings no additional information, but at the same time no longer
follow the GNU coding style: Comments should be English sentences
starting a capital letter and ending with a dot.  Not also the two
spaces after the dot, which is mandated by the GNU Coding Style.

- fprintf_filtered (file, "Status Word: %s",
- hex_string_custom (status, 4));
+ fprintf_filtered (file, "status word : %s\n", hex_string_custom(status, 4));

You made a formatting change which is incorrect: The code should go no further than column 78-80. Please leave the call to hex_string_custom on the next line where it was. Make sure, when you send a new patch, that the diff does not show any difference on this line, particularly differences in spaces.

+ fprintf_filtered (file, "%s ", (status & 0x0020) ? "PE" : " "); /* precision */

I think that these added comments are superfluous - any developer working on that code should have a basic knowledge of this FPU and thus know what these abbreviations mean. I do not mind them so much, except that they cause the line to exceed 80 chars, or be sufficient close to it, that these comments should be on a line of their own.



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