Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Andreas Ericsson <ae@op5.se>]

Pierre Habouzit wrote:
> On Wed, Sep 19, 2007 at 06:37:31AM +0000, Andreas Ericsson wrote:
>> Pierre Habouzit wrote:
>>> diff --git a/builtin-blame.c b/builtin-blame.c
>>> index e364b6c..16c0ca8 100644
>>> --- a/builtin-blame.c
>>> +++ b/builtin-blame.c
>>> @@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
>>> static void write_filename_info(const char *path)
>>> {
>>> 	printf("filename ");
>>> -	write_name_quoted(NULL, 0, path, 1, stdout);
>>> -	putchar('\n');
>>> +	write_name_quoted(path, stdout, '\n');
>>> }
>>>
>> This looks like a candidate for a macro. I'm not sure if gcc optimizes
>> sibling calls in void functions with -O2, and it doesn't inline without
>> -O3.
> 
>   Well, there is little point. write_name_quoted behaviour changes if
> the last argument is \0 or non-\0 (see patch comment and quote.c code),
> so it does not really matter to inline the "putchar" IMHO.
> 
>>> -static void diff_flush_raw(struct diff_filepair *p,
>>> -			   struct diff_options *options)
>>> +static void diff_flush_raw(struct diff_filepair *p, struct diff_options 
>>> *opt)
>> Parameter rename? I'd have thought the patch was big enough as it is ;-)
> 
>   I'm anal when it comes to code: the rule of the least surprise should
> apply, and consistency is fundamental. And it happens that diff_options
> are always called `opt' in diff.c, except in that place (and it allows
> to write the prototype of the function on one line).
> 

Then perhaps a separate patch for this would have been prudent? I'm not
against the change per se and I understand the reasoning behind it, but
it seems to go against Documentation/SubmittingPatches (submit one change
at a time).

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231