Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
- From
Andreas Ericsson <ae@op5.se>
- Date
- Sep 19, 2007, 08:08 UTC
- Message-ID
- <46F0D8E2.5090706@op5.se>
- In-Reply-To
- <20070919080030.GA28205@artemis.corp>
Pierre Habouzit wrote:
Show 34 quoted lines
> 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