Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.
On Wed, Sep 19, 2007 at 06:37:31AM +0000, Andreas Ericsson wrote:
Show 18 quoted lines
> 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.
Show 6 quoted lines
> > -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).
> Other than that, the diffstat calls this a good patch, and given the
> fact that all your previous series passed all tests, I assume this one
> does too.
Yes, before submitting a series I check the testsuite passes at each
step, so that it doesn't break git-bisect in obvious ways.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
Pierre Habouzit <madcoder@debian.org>