Re: [PATCH] attr: map builtin userdiff drivers to well-known extensions

From: Jeff King <peff@peff.net>
Date: Dec 16, 2011, 19:21 UTC
Message-ID: <20111216192104.GA19924@sigill.intra.peff.net>
In-Reply-To: <4EEB4F13.2010402@viscovery.net>

On Fri, Dec 16, 2011 at 03:00:51PM +0100, Johannes Sixt wrote:

Show 22 quoted lines

> Am 12/16/2011 12:00, schrieb Jeff King:
> >  static const char *builtin_attr[] = {
> ...
> > +	"*.c diff=cpp",
> > +	"*.cc diff=cpp",
> > +	"*.cxx diff=cpp",
> > +	"*.cpp diff=cpp",
> > +	"*.h diff=cpp",
> > +	"*.hpp diff=cpp",
> 
> Please don't do this. It would be a serious regression for C++ coders, and
> some C coders as well. The built-in hunk header patterns are severly
> broken and don't work well with C++ code. I know for sure that the
> following are not recognized:
> 
> - template declarations, e.g. template<class T> func(T x);
> - constructor definitionss, e.g. MyClass::MyClass()
> - functions that return references, e.g. const string& func()
> - function definitions along the GNU coding style, e.g.
> 
>      void
>      the_func ()

Hmm. I think it's a legitimate criticism to say "hunk-header detection is a broken feature because our heuristics aren't good enough, and we shouldn't start using it by default because people will complain because it sucks too much".

At the same time, I think we have seen people complaining that the regular dumb funcname detection is not good enough[1], and that using language-specific funcnames, while not 100% perfect, produces better results on the whole.

So I think rather than saying "this doesn't always work", it's important to ask "on the whole, does this tend to produce better results than without, and when we are wrong, how bad is it?"

I'm not clear from what you wrote on whether you were saying it is simply sub-optimal, or whether on balance it is way worse than the default funcname matching.

And if it is bad on balance, is the right solution to avoid exposing people to it, or is it to make our patterns better? I.e., is it fixable, or is it simply too hard a problem to get right in the general case, and we shouldn't turn it on by default?

Show 6 quoted lines

> I am currently using this pattern (but I'm sure it can be optimized) with
> an appropriate xcpp attribute:
> 
> [diff "xcpp"]
>         xfuncname = "!^[
> \\t]*[a-zA-Z_][a-zA-Z_0-9]*[^()]*:[[:space:]]*$\n^[a-zA-Z_][a-zA-Z_0-9]*.*"

So, I'm confused. If you are using this, surely you have "*.c diff=xcpp" in your attributes file, and my patch has no effect for you, as it is lower precedence than user-supplied gitattributes? Also, if you called it diff.cpp.xfuncname, then wouldn't my patch still be useful, as your complaint is not "my *.c files are not actually C language" but "the C language driver sucks" (but you be remedying that by providing your own config).

-Peff