Re: gdb-patch mailing list

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

Michael wrote:
> 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.
this is not mine, I think you are in error??
>>
>> 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.
>>
>>   
>
>