RFC patchdiff: simplify cpp funcname regex

The current funcname matcher for C files requires one or more words before the function name, like:

  static int foo(int arg)
  {

However, some coding styles look like this:

  static int
  foo(int arg)
  {

and we do not match, even though the default regex would.

This patch simplifies the regex significantly. Rather than trying to handle all the variants of keywords and return types, we simply look for an identifier at the start of the line that contains a "(", meaning it is either a function definition or a function call, and then not containing ";" which would indicate it is a call or declaration.

This can be fooled by something like:

    if (foo)

but it is unlikely to find that at the very start of a line with no identation (and without having a complete set of keywords, we cannot distinguish that from a function called "if" taking "foo" anyway).

We had no tests at all for .c files, so this attempts to add a few obvious cases (including the one we are fixing here).

Signed-off-by: Jeff King <peff@peff.net>
---
I tried accommodating this one case in the current regex, but it just
kept getting more complicated and unreadable. Maybe I am being naive to
think that this much simpler version can work. We don't have a lot of
tests or a known-good data set to check.

I did try "git log -1000 -p" before and after on git.git, diff'd the results and manually inspected. The results were a mix of strict improvement (mostly instances of the style I was trying to fix here) and cases where there is no good answer. For example, for top-level changes outside functions, we might find:

  N_("some text that is long"

that is part of:

  const char *foo =
  N_("some text that is long"
  "and spans multiple lines");

Before this change, we would skip past it (using the cpp regex, that is; the default one tends to find the same line) and either report nothing, or whatever random function was before us. So it's a behavior change, but the existing behavior is really no better.

 t/t4018-diff-funcname.sh | 36 ++++++++++++++++++++++++++++++++++++
 userdiff.c               |  2 +-
 2 files changed, 37 insertions(+), 1 deletion(-)

Am 3/5/2014 1:36, schrieb Jeff King:

Here is a patch that I'm carrying around since... a while. What do you think?

The pattern I chose also catches variable definition, not just functions. That is what I need, but it hurts grep --function-context That's the reason I didn't forward the patch, yet.

--- 8< ---
From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 25 Sep 2012 14:08:02 +0200
Subject: [PATCH] userdiff: have 'cpp' hunk header pattern catch more C++ anchor points

The hunk header pattern 'cpp' is intended for C and C++ source code, but it is actually not very useful for the latter, and even hurts some use-cases for the former.

The parts of the pattern have the following flaws:

- The first part matches an identifier followed immediately by a colon and
  arbitrary text and is intended to reject goto labels and C++ access
  specifiers (public, private, protected). But this pattern also rejects
  C++ constructs, which look like this:

    MyClass::MyClass()
    MyClass::~MyClass()
    MyClass::Item MyClass::Find(...

- The second part matches an identifier followed by a list of qualified
  names (i.e. identifiers separated by the C++ scope operator '::')
  separated by space or '*' followed by an opening parenthesis (with space
  between the tokens). It matches function declarations like

    struct item* get_head(...
    int Outer::Inner::Func(...

  Since the pattern requires at least two identifiers, GNU-style function
  definitions are ignored:

    void
    func(...

  Moreover, since the pattern does not allow punctuation other than '*',
  the following C++ constructs are not recognized:

  . template definitions:
      template<class T> int func(T arg)

  . functions returning references:
      const string& get_message()

  . functions returning templated types:
      vector<int> foo()

  . operator definitions:
      Value operator+(Value l, Value r)

- The third part of the pattern finally matches compound definitions. But
  it forgets about unions and namespaces, and also skips single-line
  definitions

    struct random_iterator_tag {};

  because no semicolon can occur on the line.

Change the first pattern to require a colon at the end of the line (except for trailing space and comments), so that it does not reject constructor or destructor definitions.

Notice that all interesting anchor points begin with an identifier or keyword. But since there is a large variety of syntactical constructs after the first "word", the simplest is to require only this word and accept everything else. Therefore, this boils down to a line that begins with a letter or underscore (optionally preceded by the C++ scope operator '::' to accept functions returning a type anchored at the global namespace). Replace the second and third part by a single pattern that picks such a line.

This has the following desirable consequence:

- All constructs mentioned above are recognized.

and the following likely desirable consequences:

- Definitions of global variables and typedefs are recognized:

    int num_entries = 0;
    extern const char* help_text;
    typedef basic_string<wchar_t> wstring;

- Commonly used marco-ized boilerplate code is recognized:

    BEGIN_MESSAGE_MAP(CCanvas,CWnd)
    Q_DECLARE_METATYPE(MyStruct)
    PATTERNS("tex",...)

  (The last one is from this very patch.)

but also the following possibly undesirable consequence:

- When a label is not on a line by itself (except for a comment) it is no
  longer rejected, but can appear as a hunk header if it occurs at the
  beginning of a line:

    next:;

IMO, the benefits of the change outweigh the (possible) regressions by a large margin.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 userdiff.c                                 | 8 +++-----
 13 files changed, 3 insertions(+), 17 deletions(-)

On Wed, Mar 05, 2014 at 08:58:26AM +0100, Johannes Sixt wrote:

If by variable definition you mean:

   struct foo bar = {
  -       old
  +       new
   };

I'd think that would be covered by the existing "struct|class|enum". Though I think we'd want to also allow keywords in front of it, like "static". I suspect the original was more meant to find:

   struct foo {
  -old
  +new
   };

Makes sense. I noticed your fix is to look for end-of-line or comments afterwards. Would it be simpler to just check for a non-colon, like:

  !^[ \t]*[A-Za-z_][A-Za-z_0-9]*:($|[^:])

A tried to keep the "looks like a function definition" bit in mine, and yours loosens this quite a bit more. I think that may be OK. That is, I do not think there is any reason for somebody to do:

    void foo() {
    call_to_bar();
   -old
   +new
    }

That is, nobody would put a function _call_ without indentation. If something has alphanumerics at the left-most column, then it is probably interesting no matter what.

I don't see how that is an interesting line. The point is to find a block that is surrounding the changes, but that is not surrounding the lines below.

Yeah, this bit makes sense to me.

Both yours and mine will find the first line here in things like:

   void foo(void);
  -void bar(void);
  +void bar(int arg);

but I think that is OK. There _isn't_ any interesting surrounding context here. The current code will sometimes come up with an empty funcline (which is good), but it may just as easily come up with a totally bogus funcline in a case like:

   void unrelated(void)
   {
   }

   void foo(void);
  -void bar(void);
  +void bar(int arg);

So trying to be very restrictive and say "that doesn't look like a function" does not really buy us anything (and it creates tons of false negatives, as you documented, because C++ syntax has all kinds of crazy stuff).

_If_ the backwards search learned to terminate (e.g., seeing the "^}" line and saying "well, we can't be inside a function"), then such negative lines might be useful for coming up with an empty funcname rather than the bogus "void foo(void);". But we do not do that currently, and I do not think it is that useful (the funcname above is arguably just as or more useful than an empty one).

-Peff

Am 3/6/2014 22:28, schrieb Jeff King:

No, I meant lines like

    static double var;
   -static int old;
   +static int new;

The motivation is to show hints where in a file the change is located: Anything that could be of significance for the author should be picked up.

But that does not necessarily help grep --function-context. For example, when there is a longish block of global variable definitions and there is a match in the middle, then --function-context would provide no context because the line itself would be regarded as the beginning of a "function", i.e., the context, and the next line (which also matches the pattern) would be the beginning of the *next* function, and would not be in the context anymore.

I want to match [[:space:]] after the label's colon, because I have lot's of C++ files with CRLF, and I need to match the CR. Your more liberal pattern would fit as well, although it would pick up a bit field as in

   struct foo {
      unsigned
        flag: 1;
   -old
   +new

I would not mind ignoring this case ("garbage in, garbage out" ;-).

I more often than not want to have an answer to the question "where?", not "wherein?" Then anything that helps locate a hunk is useful.

(The particular example, an empty struct, looks strange for C programmers, of course, but it's a common idiom in C++ when it comes to template meta-programming.)

As I said, my motivation is not so much to find a "container", but rather a clue to help locate a change while reading the patch text. I can speak for myself, but I have no idea what is more important for the majority.

-- Hannes

On Fri, Mar 07, 2014 at 08:23:05AM +0100, Johannes Sixt wrote:

I see. Coupled with what you said below:

your proposal makes a lot more sense to me, and I think that is really at the center of our discussion. I do not have a gut feeling for which is "more right" (i.e., "container" versus "context").

But given that most of the cases we are discussing are ones where the "diff -p" default regex gets it right (or at least better than) compared to the C regex, I am tempted to say that we should be erring in the direction of simplicity, and just finding interesting lines without worrying about containers (i.e., it argues for your patch).

Thanks, I was having trouble thinking of another good use of a colon, but bitfields are what I was missing. Your pattern is probably better here.

So I am leaning towards your patch, but I'm not sure if I understand all of the implications for "git grep". Can you give some concrete examples?

-Peff

Am 3/14/2014 4:54, schrieb Jeff King:

We are in agreement here. So, let's base a decision on the implications on git grep.

Consider this code:

  void above()
  {}
  static int Y;
  static int A;
  int bar()
  {
    return X;
  }
  void below()
  {}

When you 'git grep --function-context X', then you get this output with the current pattern, you proposal, and my proposal (file name etc omitted for brevity):

  int bar()
  {
    return X;
  }

because we start the context at the last hunk header pattern above *or including the matching line* (and write it in the output), and we stop at the next hunk header pattern below the match (and do not write it in the output).

When you 'git grep --function-context Y', what do you want to see? With the current pattern, and with your pattern that forbids semicolon we get:

  void above()
  {}
  static int Y;
  static int A;

and with my simple pattern, which allows semicolon, we get merely

  static int Y;

because the line itself is a hunk header (and we do not look back any further) and the next line is as well. That is not exactly "function context", and that is what I'm a bit worried about.

-- Hannes

On Fri, Mar 14, 2014 at 07:56:46AM +0100, Johannes Sixt wrote:

Thanks, this example is very helpful.

Right, that makes sense to me.

Hmm. To be honest, I do not see yours as all that bad. Is "above()" or "A" actually interesting here? I'm not sure that they are. But then I do not use --function-context myself.

I guess it violates the "show things that are vaguely nearby, rather than a container" view of context that we discussed earlier. But somehow that seems less important to me with "--function-context".

So I dunno. I kind of like your version.

-Peff

Cc René; do you have any comments regarding grep --function-context?

Am 3/18/2014 6:24, schrieb Jeff King:

Then I'll polish my patch series (it also rewrites the test framework) and post it.

-- Hannes

Am 18.03.2014 09:02, schrieb Johannes Sixt:

In global context there is no "function context", of course, so the latter makes sense.

"grep --function-context" is about useful context and its implementation piggy-backs on the hunk header definitions. If those are useful then the grep output should be fine as well. IAW: No worries, go ahead. :)

However, I only use the defaults heuristic (which shows just the Y-line as well) and don't know C++, so I my opinion on this matter isn't worth that much.

René

Here is a series that makes the hunk header pattern for C and C++ even simpler than suggested by Peff in [1] to catch a lot more C++ functions and two more C patterns.

As a preparatory work, the test cases are totally rewritten to make it a lot simpler to drop in new tests. There was an earlier attempt to change the infrastructure [2], and it is the reason for the widened Cc list.

Two patches also extend the word regexp.

[1] http://article.gmane.org/gmane.comp.version-control.git/243408 [2] http://thread.gmane.org/gmane.comp.version-control.git/187269/focus=187497

Johannes Sixt (10):
  userdiff: support C++ ->* and .* operators in the word regexp
  userdiff: support unsigned and long long suffixes of integer constants
  t4018: an infrastructure to test hunk headers
  t4018: convert perl pattern tests to the new infrastructure
  t4018: convert java pattern test to the new infrastructure
  t4018: convert custom pattern test to the new infrastructure
  t4018: reduce test files for pattern compilation tests
  t4018: test cases for the built-in cpp pattern
  t4018: test cases showing that the cpp pattern misses many anchor
    points
  userdiff: have 'cpp' hunk header pattern catch more C++ anchor points

 t/t4018-diff-funcname.sh                   | 230 ++++++++++-------------------
 t/t4018/README                             |  18 +++
 t/t4018/cpp-c++-function                   |   4 +
 t/t4018/cpp-class-constructor              |   4 +
 t/t4018/cpp-class-constructor-mem-init     |   5 +
 t/t4018/cpp-class-definition               |   4 +
 t/t4018/cpp-class-definition-derived       |   5 +
 t/t4018/cpp-class-destructor               |   4 +
 t/t4018/cpp-function-returning-global-type |   4 +
 t/t4018/cpp-function-returning-nested      |   5 +
 t/t4018/cpp-function-returning-pointer     |   4 +
 t/t4018/cpp-function-returning-reference   |   4 +
 t/t4018/cpp-gnu-style-function             |   5 +
 t/t4018/cpp-namespace-definition           |   4 +
 t/t4018/cpp-operator-definition            |   4 +
 t/t4018/cpp-skip-access-specifiers         |   8 +
 t/t4018/cpp-skip-comment-block             |   9 ++
 t/t4018/cpp-skip-labels                    |   8 +
 t/t4018/cpp-struct-definition              |   9 ++
 t/t4018/cpp-struct-single-line             |   7 +
 t/t4018/cpp-template-function-definition   |   4 +
 t/t4018/cpp-union-definition               |   4 +
 t/t4018/cpp-void-c-function                |   4 +
 t/t4018/custom1-pattern                    |  17 +++
 t/t4018/custom2-match-to-end-of-line       |   8 +
 t/t4018/custom3-alternation-in-pattern     |  17 +++
 t/t4018/java-class-member-function         |   8 +
 t/t4018/perl-skip-end-of-heredoc           |   8 +
 t/t4018/perl-skip-forward-decl             |  10 ++
 t/t4018/perl-skip-sub-in-pod               |  18 +++
 t/t4018/perl-sub-definition                |   4 +
 t/t4018/perl-sub-definition-kr-brace       |   4 +
 userdiff.c                                 |  12 +-
 33 files changed, 303 insertions(+), 160 deletions(-)
 create mode 100644 t/t4018/README
 create mode 100644 t/t4018/cpp-c++-function
 create mode 100644 t/t4018/cpp-class-constructor
 create mode 100644 t/t4018/cpp-class-constructor-mem-init
 create mode 100644 t/t4018/cpp-class-definition
 create mode 100644 t/t4018/cpp-class-definition-derived
 create mode 100644 t/t4018/cpp-class-destructor
 create mode 100644 t/t4018/cpp-function-returning-global-type
 create mode 100644 t/t4018/cpp-function-returning-nested
 create mode 100644 t/t4018/cpp-function-returning-pointer
 create mode 100644 t/t4018/cpp-function-returning-reference
 create mode 100644 t/t4018/cpp-gnu-style-function
 create mode 100644 t/t4018/cpp-namespace-definition
 create mode 100644 t/t4018/cpp-operator-definition
 create mode 100644 t/t4018/cpp-skip-access-specifiers
 create mode 100644 t/t4018/cpp-skip-comment-block
 create mode 100644 t/t4018/cpp-skip-labels
 create mode 100644 t/t4018/cpp-struct-definition
 create mode 100644 t/t4018/cpp-struct-single-line
 create mode 100644 t/t4018/cpp-template-function-definition
 create mode 100644 t/t4018/cpp-union-definition
 create mode 100644 t/t4018/cpp-void-c-function
 create mode 100644 t/t4018/custom1-pattern
 create mode 100644 t/t4018/custom2-match-to-end-of-line
 create mode 100644 t/t4018/custom3-alternation-in-pattern
 create mode 100644 t/t4018/java-class-member-function
 create mode 100644 t/t4018/perl-skip-end-of-heredoc
 create mode 100644 t/t4018/perl-skip-forward-decl
 create mode 100644 t/t4018/perl-skip-sub-in-pod
 create mode 100644 t/t4018/perl-sub-definition
 create mode 100644 t/t4018/perl-sub-definition-kr-brace

The character sequences ->* and .* are valid C++ operators. Keep them together in --word-diff mode.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 userdiff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Do not split constants such as 123U, 456ll, 789UL at the first U or second L.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 userdiff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Add an infrastructure that simplifies adding new tests of the hunk header regular expressions.

To add new tests, a file with the syntax to test can be dropped in the directory t4018. The README file explains how a test file must contain; the README itself tests the default behavior.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018-diff-funcname.sh | 60 +++++++++++++++++++++++++++++++++++++++++++-----
 t/t4018/README           | 18 +++++++++++++++
 2 files changed, 72 insertions(+), 6 deletions(-)
 create mode 100644 t/t4018/README

Johannes Sixt <j6t@kdbg.org> writes:

s/how/what/, or "how a test file must be written" you mean?

Thanks.  Looks like a reasonable way to mark what must be found.

I always found this "Let's apply rules for language $p to these *.java files" strange. I have wonder if it makes sense to further change the framework to read the name of the rule to be applied from the file in t/t4018/ directory, instead of using filename that is the same as the name of the rule? That way, you can list the files in t/t4018/ directory to come up with the above list, without having to maintain the list of rules separately like the above.

I like that you can have more than one test for each language/rule this way, allowing you to test one kind of breakage without getting affected by lines prepared for other tests in the same file.

Am 21.03.2014 23:00, schrieb Junio C Hamano:

I do mean one of those, preferably the latter ;)

I see what you mean. Notice, however, that we also test whether the regular expressions can be compiled successfully, and for this it is desirable to have the complete list of userdiff drivers. Until we have at least one test-case for each driver, we wouldn't get the complete list. In summary, I do agree that a bit more automation is also desirable, but we are not there, yet.

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

I missed the fact that we lack "here is a pair of test files in this lang $p; go find diff between them and make sure we have the right hunk header" for many $p.

So the kind of improvements I wondered about are allowed to come later, but shouldn't be done in this series, especiallly not at this step in the series.

Thanks.

On Fri, Mar 21, 2014 at 10:07:15PM +0100, Johannes Sixt wrote:

I really like the cleanups you've done in t4018. I noticed how messy it was when I modified it recently, but I didn't take the time to clean it.

The existing tests use -U1 to make writing cases simpler. Is there a reason not to continue that (or if you found that porting the existing cases was not a chore with -U3, I can buy that argument, too)?

I wondered why you wouldn't write them in the case you are indicating, when...

Ah. Clever. :)

-Peff

On Mon, Mar 24, 2014 at 05:36:59PM -0400, Jeff King wrote:

I take it back. You did keep "-U1" in the result. Is this "two lines" rule necessary, then?

-Peff

Am 24.03.2014 22:39, schrieb Jeff King:

When we have

   one
   two
   three

how would you describe the distance between "one" and "three"?

Or do you have a wrong expectation how we determine the hunk header? The hunk header is searched in the text *before* the pre-context. Therefore, we need at least as many lines between the intended hunk header line (with tag RIGHT) and the changed line (ChangeMe) as we request with -U for the context.

So: yes, we need the rule.

-- Hannes

On Tue, Mar 25, 2014 at 09:07:18PM +0100, Johannes Sixt wrote:

I read it as "two lines between". Though that would be appropriate for -U2, so I think I probably had an off-by-one in my head, then, too.

So it is probably fine as-is, and this was mostly just me confusing myself. Sorry for the noise.

-Peff

There is one subtlety: The old test case 'perl pattern gets full line of POD header' does not have its own new test case, but the feature is tested nevertheless by placing the RIGHT tag at the end of the expected hunk header in t4018/perl-skip-sub-in-pod.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018-diff-funcname.sh             | 88 ------------------------------------
 t/t4018/perl-skip-end-of-heredoc     |  8 ++++
 t/t4018/perl-skip-forward-decl       | 10 ++++
 t/t4018/perl-skip-sub-in-pod         | 18 ++++++++
 t/t4018/perl-sub-definition          |  4 ++
 t/t4018/perl-sub-definition-kr-brace |  4 ++
 6 files changed, 44 insertions(+), 88 deletions(-)
 create mode 100644 t/t4018/perl-skip-end-of-heredoc
 create mode 100644 t/t4018/perl-skip-forward-decl
 create mode 100644 t/t4018/perl-skip-sub-in-pod
 create mode 100644 t/t4018/perl-sub-definition
 create mode 100644 t/t4018/perl-sub-definition-kr-brace

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018-diff-funcname.sh           | 4 ----
 t/t4018/java-class-member-function | 8 ++++++++
 2 files changed, 8 insertions(+), 4 deletions(-)
 create mode 100644 t/t4018/java-class-member-function

For the test case "matches to end of line", extend the pattern by a few wildcards so that the pattern captures the "RIGHT" token, which is needed for verification, without mentioning it in the pattern.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018-diff-funcname.sh               | 40 ++++++++++++++--------------------
 t/t4018/custom1-pattern                | 17 +++++++++++++++
 t/t4018/custom2-match-to-end-of-line   |  8 +++++++
 t/t4018/custom3-alternation-in-pattern | 17 +++++++++++++++
 4 files changed, 58 insertions(+), 24 deletions(-)
 create mode 100644 t/t4018/custom1-pattern
 create mode 100644 t/t4018/custom2-match-to-end-of-line
 create mode 100644 t/t4018/custom3-alternation-in-pattern

All test cases that need a file with specific text patterns have been converted to utilize texts in the t4018/ directory. The remaining tests in the test script deal only with the validity of the regular expressions. These tests do not depend on the contents of files that 'git diff' is invoked on. Remove the largish here-document and use only tiny files.

While we are touching these tests, convert grep to test_i18ngrep as the texts checked for may undergo translation in the future.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018-diff-funcname.sh | 52 +++++++++++++-----------------------------------
 1 file changed, 14 insertions(+), 38 deletions(-)

A later patch changes the built-in cpp pattern. These test cases demonstrate aspects of the pattern that we do not want to change.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018/cpp-c++-function               | 4 ++++
 t/t4018/cpp-class-definition           | 4 ++++
 t/t4018/cpp-class-definition-derived   | 5 +++++
 t/t4018/cpp-function-returning-pointer | 4 ++++
 t/t4018/cpp-skip-access-specifiers     | 8 ++++++++
 t/t4018/cpp-skip-comment-block         | 9 +++++++++
 t/t4018/cpp-skip-labels                | 8 ++++++++
 t/t4018/cpp-struct-definition          | 9 +++++++++
 t/t4018/cpp-void-c-function            | 4 ++++
 9 files changed, 55 insertions(+)
 create mode 100644 t/t4018/cpp-c++-function
 create mode 100644 t/t4018/cpp-class-definition
 create mode 100644 t/t4018/cpp-class-definition-derived
 create mode 100644 t/t4018/cpp-function-returning-pointer
 create mode 100644 t/t4018/cpp-skip-access-specifiers
 create mode 100644 t/t4018/cpp-skip-comment-block
 create mode 100644 t/t4018/cpp-skip-labels
 create mode 100644 t/t4018/cpp-struct-definition
 create mode 100644 t/t4018/cpp-void-c-function

Most of the tests show C++ code, but there is also a union definition and a GNU style function definition that are not recognized.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018/cpp-class-constructor              | 5 +++++
 t/t4018/cpp-class-constructor-mem-init     | 6 ++++++
 t/t4018/cpp-class-destructor               | 5 +++++
 t/t4018/cpp-function-returning-global-type | 5 +++++
 t/t4018/cpp-function-returning-nested      | 6 ++++++
 t/t4018/cpp-function-returning-reference   | 5 +++++
 t/t4018/cpp-gnu-style-function             | 6 ++++++
 t/t4018/cpp-namespace-definition           | 5 +++++
 t/t4018/cpp-operator-definition            | 5 +++++
 t/t4018/cpp-struct-single-line             | 8 ++++++++
 t/t4018/cpp-template-function-definition   | 5 +++++
 t/t4018/cpp-union-definition               | 5 +++++
 12 files changed, 66 insertions(+)
 create mode 100644 t/t4018/cpp-class-constructor
 create mode 100644 t/t4018/cpp-class-constructor-mem-init
 create mode 100644 t/t4018/cpp-class-destructor
 create mode 100644 t/t4018/cpp-function-returning-global-type
 create mode 100644 t/t4018/cpp-function-returning-nested
 create mode 100644 t/t4018/cpp-function-returning-reference
 create mode 100644 t/t4018/cpp-gnu-style-function
 create mode 100644 t/t4018/cpp-namespace-definition
 create mode 100644 t/t4018/cpp-operator-definition
 create mode 100644 t/t4018/cpp-struct-single-line
 create mode 100644 t/t4018/cpp-template-function-definition
 create mode 100644 t/t4018/cpp-union-definition

The hunk header pattern 'cpp' is intended for C and C++ source code, but it is actually not particularly useful for the latter, and even misses some use-cases for the former.

The parts of the pattern have the following flaws:

- The first part matches an identifier followed immediately by a colon
  and arbitrary text and is intended to reject goto labels and C++
  access specifiers (public, private, protected). But this pattern also
  rejects C++ constructs, which look like this:

    MyClass::MyClass()
    MyClass::~MyClass()
    MyClass::Item MyClass::Find(...

- The second part matches an identifier followed by a list of qualified
  names (i.e. identifiers separated by the C++ scope operator '::')
  separated by space or '*' followed by an opening parenthesis (with
  space between the tokens). It matches function declarations like

    struct item* get_head(...
    int Outer::Inner::Func(...

  Since the pattern requires at least two identifiers, GNU-style
  function definitions are ignored:

    void
    func(...

  Moreover, since the pattern does not allow punctuation other than '*',
  the following C++ constructs are not recognized:

  . template definitions:
      template<class T> int func(T arg)

  . functions returning references:
      const string& get_message()

  . functions returning templated types:
      vector<int> foo()

  . operator definitions:
      Value operator+(Value l, Value r)

- The third part of the pattern finally matches compound definitions.
  But it forgets about unions and namespaces, and also skips single-line
  definitions

    struct random_iterator_tag {};

  because no semicolon can occur on the line.

Change the first pattern to require a colon at the end of the line (except for trailing space and comments), so that it does not reject constructor or destructor definitions.

Notice that all interesting anchor points begin with an identifier or keyword. But since there is a large variety of syntactical constructs after the first "word", the simplest is to require only this word and accept everything else. Therefore, this boils down to a line that begins with a letter or underscore (optionally preceded by the C++ scope operator '::' to accept functions returning a type anchored at the global namespace). Replace the second and third part by a single pattern that picks such a line.

This has the following desirable consequence:

- All constructs mentioned above are recognized.

and the following likely desirable consequences:

- Definitions of global variables and typedefs are recognized:

    int num_entries = 0;
    extern const char* help_text;
    typedef basic_string<wchar_t> wstring;

- Commonly used marco-ized boilerplate code is recognized:

    BEGIN_MESSAGE_MAP(CCanvas,CWnd)
    Q_DECLARE_METATYPE(MyStruct)
    PATTERNS("tex",...)

  (The last one is from this very patch.)

but also the following possibly undesirable consequence:

- When a label is not on a line by itself (except for a comment) it is
  no longer rejected, but can appear as a hunk header if it occurs at
  the beginning of a line:

    next:;

IMO, the benefits of the change outweigh the (possible) regressions by a large margin.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t4018/cpp-class-constructor              | 1 -
 t/t4018/cpp-class-constructor-mem-init     | 1 -
 t/t4018/cpp-class-destructor               | 1 -
 t/t4018/cpp-function-returning-global-type | 1 -
 t/t4018/cpp-function-returning-nested      | 1 -
 t/t4018/cpp-function-returning-reference   | 1 -
 t/t4018/cpp-gnu-style-function             | 1 -
 t/t4018/cpp-namespace-definition           | 1 -
 t/t4018/cpp-operator-definition            | 1 -
 t/t4018/cpp-struct-single-line             | 1 -
 t/t4018/cpp-template-function-definition   | 1 -
 t/t4018/cpp-union-definition               | 1 -
 userdiff.c                                 | 8 +++-----
 13 files changed, 3 insertions(+), 17 deletions(-)

Thanks; will replace jk/diff-funcname-cpp-regex with this series.

On Fri, Mar 21, 2014 at 10:07:12PM +0100, Johannes Sixt wrote:

Thanks. This looks sane overall, and I am especially happy about the increased test coverage. I do not know if your 10/10 misses some cases, but at least now we can collect and reason about a set of examples going forward.

-Peff

Jeff King <peff@peff.net> writes:

It is not worth worrying about:

foo(arg,
    another);

that is not indented, so I think that simplicity is good.

Unfortunate, but cannot be avoided.

True.