Re: [PATCH v2 2/2] Makefile: add style build rule
- From
Jeff King <peff@peff.net>
- Date
- Aug 15, 2017, 01:52 UTC
- Message-ID
- <20170815015256.c423wjwrq7crghu4@sigill.intra.peff.net>
- In-Reply-To
- <xmqqvalpsoda.fsf@gitster.mtv.corp.google.com>
On Mon, Aug 14, 2017 at 04:47:45PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> > By the way, I do not know which vintage of /usr/bin/git-clang-format > > I happen to have on my box, but I needed a crude workaround patch > > (attached at the end) ... > > I guess you hit the same thing while our messages crossing ;-)
Yep. Our solutions were at opposite ends of the spectrum, though. :)
Show 33 quoted lines
> > As to what it does, the first example I tried may not have been a
> > great one. I got this:
> >
> > git clang-format --style file --diff --extensions c,h
> > diff --git a/cache.h b/cache.h
> > index 73e0085186..6462fe25bc 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -1498,11 +1498,8 @@ struct checkout {
> > const char *base_dir;
> > int base_dir_len;
> > struct delayed_checkout *delayed_checkout;
> > - unsigned force:1,
> > - quiet:1,
> > - not_new:1,
> > - a_new_field:1,
> > - refresh_cache:1;
> > + unsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,
> > + refresh_cache : 1;
> > };
> > #define CHECKOUT_INIT { NULL, "" }
> >
> > which is not wrong per-se, but I have a mixed feelings. I do not
> > want it to complain if the original tried to fit many items on a
> > single line, but if the original wanted to have one item per line,
> > I'd rather see it kept as-is.
>
> To clarify, the above is after I added a_new_field that is one-bit
> wide without doing anything else. I do not mind the checker
> complaining the existing force, quiet, etc. whose widths are all
> spelled without SP around ':', because they appear near-by, as a
> collateral damage. My only gripe is that the result got squished
> into a single line.Yes, agreed. My personal rule with a list like this is often "once you have to start breaking across multiple lines, you should put one per line". I don't know if there's a way to codify that in clang-format, though.
The case I fed it (which is just nonsense I made up that does not fit our style) also left me a bit confused at first, but I think it was because the .clang-format parser was bailing as soon as it found an unrecognized entry, but then formatting according to bogus rules. With the original file from Brandon I got:
diff --git a/builtin/merge-file.c b/builtin/merge-file.c index 7e8371670b..8994450e0c 100644 --- a/builtin/merge-file.c +++ b/builtin/merge-file.c @@ -21,10 +21,7 @@ static int label_cb(const struct option *opt, const char *arg, int unset) return 0; } -static -int foo(void* bar,int baz) { - /* nothing */ -} +static int foo(void *bar, int baz) { /* nothing */ } int cmd_merge_file(int argc, const char **argv, const char *prefix) { which is clearly not our style. And then after removing the entries I mentioned elsewhere, I get: diff --git a/builtin/merge-file.c b/builtin/merge-file.c index 7e8371670b..574ba6d86f 100644 --- a/builtin/merge-file.c +++ b/builtin/merge-file.c @@ -21,8 +21,8 @@ static int label_cb(const struct option *opt, const char *arg, int unset) return 0; } -static -int foo(void* bar,int baz) { +static int foo(void *bar, int baz) +{ /* nothing */ } which looks right. So you might want to double-check that it was respecting our settings, and there were no warnings to stderr. -Peff