Re: gdb-patch mailing list

[Michael <gdb-patches@cyberfiber.org>]

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.
>
>