Re: gdb-patch mailing list

Michael
2010-01-02T08:48:13Z

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