Re: [PATCH] difftool.c: mark a file-local symbol with static

From: Jeff King <peff@peff.net>
Date: Dec 1, 2016, 18:50 UTC
Message-ID: <20161201185056.eso5rhec7izlbywa@sigill.intra.peff.net>
In-Reply-To: <xmqq60n3bjel.fsf@gitster.mtv.corp.google.com>

On Thu, Dec 01, 2016 at 10:20:50AM -0800, Junio C Hamano wrote:

Show 27 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I don't have a preference on which direction we go, but yes, right now
> > we are in an awkward middle ground. We should do one of:
> >
> >   1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure
> >      future patches to do not violate it.
> >
> >   2. Declare warning("") as OK.
> >
> > I still think the warning is silly, but (1) has value in that it
> > produces the least surprise and annoyance to various people building
> > Git.
> 
> What is most awkward is that "make", with no customization out of
> box, suggests to use -Wall in CFLAGS and triggers the misguided
> warning, and DEVELOPER_CFLAGS, partly because it adds -Werror to
> turn warnings (including this misguided one) into errors, disables
> it with -Wno-format-zero-length.  As a result:
> 
>  - Casual builders that follow our suggestion get warnings.
> 
>  - Developers do not notice their new code may make the "casual
>    builder" experience worse.
> 
> That is totally backwards.  That is probably what you meant by "the
> least surprise and annoyance"?

Yes, exactly. :)

The surprise and annoyance for (1) is that people who get caught up by the warning while writing new patches say "this warning is stupid, why don't we disable it". But that is a much smaller number of people to be annoyed.

Show 7 quoted lines

> I also still think that any_printf_like_function("%s", "") looks
> silly.  I know that we've already started moving in that direction
> and we stopped at a place we do not want to be in, but perhaps it
> was a mistake to move in that direction in the first place.  I am
> tempted to say we should do something like the attached, but that
> may not fly well, as I suspect that -Wno-format-zero-length may be a
> lot more exotic than the -Wall compiler option.

Yeah, I think portability concerns are what caused us not to go down that road in the first place. But I think it's also a mistake to put too much into CFLAGS, because somebody who wants to override one flag has to repeat the rest. So, e.g., right now I might do:

  make CFLAGS="-O3 -Wall"

to bump the optimization level. But if our codebase relies on -Wno-format-zero-length being there, then I have to know to put it in there, too.

You can work around that with an extra make variable, but that also makes it harder to disable if your compiler doesn't like it.

> An obvious second
> best option would be to drop -Wall from the "everybody" CFLAGS and
> move it to DEVELOPER_CFLAGS instead.

Yeah, though that doesn't help the example above.

As ugly as warning("%s", "") is, I think it may be the thing that annoys the smallest number of people.

-Peff