threads / patch / 29095

patchuserdiff: allow * between cpp funcname words

Subject: [PATCH] userdiff: allow * between cpp funcname words

## tl;dr

9 messages between Dec 6, 2011 and Dec 7, 2011. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Thomas Rast· Dec 6, 2011, 16:35 UTC · lore

The cpp pattern, used for C and C++, would not match the start of a declaration such as

  static char *prepare_index(int argc,

because it did not allow for * anywhere between the various words that constitute the modifiers, type and function name. Fix it.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

This is a really sneaky one-character bug that I cannot believe went unnoticed for so long, seeing as there are plenty of instances within git itself where it matters.

 userdiff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to userdiff.c +1 −1
diff --git a/userdiff.c b/userdiff.c
index 7c983c1..76109da 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -118,7 +118,7 @@
 	 /* Jump targets or access declarations */
 	 "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
 	 /* C/++ functions/methods at top level */
-	 "^([A-Za-z_][A-Za-z_0-9]*([ \t]+[A-Za-z_][A-Za-z_0-9]*([ \t]*::[ \t]*[^[:space:]]+)?){1,}[ \t]*\\([^;]*)$\n"
+	 "^([A-Za-z_][A-Za-z_0-9]*([ \t*]+[A-Za-z_][A-Za-z_0-9]*([ \t]*::[ \t]*[^[:space:]]+)?){1,}[ \t]*\\([^;]*)$\n"
 	 /* compound type at top level */
 	 "^((struct|class|enum)[^;]*)$",
 	 /* -- */

-- 
1.7.8.424.gcb840
Jeff King· Dec 6, 2011, 19:02 UTC · re: Thomas Rast · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

On Tue, Dec 06, 2011 at 05:35:08PM +0100, Thomas Rast wrote:
Show 14 quoted lines
> The cpp pattern, used for C and C++, would not match the start of a
> declaration such as
> 
>   static char *prepare_index(int argc,
> 
> because it did not allow for * anywhere between the various words that
> constitute the modifiers, type and function name.  Fix it.
> 
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
> 
> This is a really sneaky one-character bug that I cannot believe went
> unnoticed for so long, seeing as there are plenty of instances within
> git itself where it matters.
Looks reasonable to me. You can see the difference, for instance, with:
  git show -U1 3c73a1d

(The -U1 is because of the annoying "we will start looking for the header at the top of context, not the top of changes" behavior I mentioned last week).

-Peff
Thomas Rast· Dec 6, 2011, 20:17 UTC · re: Jeff King · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

Jeff King wrote:
Show 24 quoted lines
> On Tue, Dec 06, 2011 at 05:35:08PM +0100, Thomas Rast wrote:
> 
> > The cpp pattern, used for C and C++, would not match the start of a
> > declaration such as
> > 
> >   static char *prepare_index(int argc,
> > 
> > because it did not allow for * anywhere between the various words that
> > constitute the modifiers, type and function name.  Fix it.
> > 
> > Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> > ---
> > 
> > This is a really sneaky one-character bug that I cannot believe went
> > unnoticed for so long, seeing as there are plenty of instances within
> > git itself where it matters.
> 
> Looks reasonable to me. You can see the difference, for instance, with:
> 
>   git show -U1 3c73a1d
> 
> (The -U1 is because of the annoying "we will start looking for the
> header at the top of context, not the top of changes" behavior I
> mentioned last week).

Actually (sadly) I'll have to revise it. It doesn't match much of C++ either, and I haven't yet come up with a reasonable regex that matches, say,

  foo::Bar<int>::t& Baz::operator<<(
which I would call ludicrous, but it's valid C++.
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Jeff King· Dec 6, 2011, 20:19 UTC · re: Thomas Rast · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
Show 15 quoted lines
> > Looks reasonable to me. You can see the difference, for instance, with:
> > 
> >   git show -U1 3c73a1d
> > 
> > (The -U1 is because of the annoying "we will start looking for the
> > header at the top of context, not the top of changes" behavior I
> > mentioned last week).
> 
> Actually (sadly) I'll have to revise it.  It doesn't match much of C++
> either, and I haven't yet come up with a reasonable regex that
> matches, say,
> 
>   foo::Bar<int>::t& Baz::operator<<(
> 
> which I would call ludicrous, but it's valid C++.

Ick, yeah. Maybe it is worth doing the "*" thing for now, and then worrying about advanced C++ stuff on top as another patch. AFAICT, your original patch is a strict improvement.

-Peff
Johannes Sixt· Dec 6, 2011, 20:52 UTC · re: Jeff King · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

Am 06.12.2011 21:19, schrieb Jeff King:
Show 21 quoted lines
> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
> 
>>> Looks reasonable to me. You can see the difference, for instance, with:
>>>
>>>   git show -U1 3c73a1d
>>>
>>> (The -U1 is because of the annoying "we will start looking for the
>>> header at the top of context, not the top of changes" behavior I
>>> mentioned last week).
>>
>> Actually (sadly) I'll have to revise it.  It doesn't match much of C++
>> either, and I haven't yet come up with a reasonable regex that
>> matches, say,
>>
>>   foo::Bar<int>::t& Baz::operator<<(
>>
>> which I would call ludicrous, but it's valid C++.
> 
> Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
> worrying about advanced C++ stuff on top as another patch. AFAICT, your
> original patch is a strict improvement.
Excuse me, where's the problem? The above example shows this
Show changes to diff +1 −2
@@ -105,8 +105,8 @@ char *url_decode(const char *url)
        struct strbuf out = STRBUF_INIT;
-       const char *slash = strchr(url, '/');
+       const char *colon = strchr(url, ':');
...

with current 4cb5d10b. This looks quite correct, no?

-- Hannes
René Scharfe· Dec 6, 2011, 21:07 UTC · re: Johannes Sixt · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

Am 06.12.2011 21:52, schrieb Johannes Sixt:
Show 32 quoted lines
> Am 06.12.2011 21:19, schrieb Jeff King:
>> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
>>
>>>> Looks reasonable to me. You can see the difference, for instance, with:
>>>>
>>>>    git show -U1 3c73a1d
>>>>
>>>> (The -U1 is because of the annoying "we will start looking for the
>>>> header at the top of context, not the top of changes" behavior I
>>>> mentioned last week).
>>>
>>> Actually (sadly) I'll have to revise it.  It doesn't match much of C++
>>> either, and I haven't yet come up with a reasonable regex that
>>> matches, say,
>>>
>>>    foo::Bar<int>::t&  Baz::operator<<(
>>>
>>> which I would call ludicrous, but it's valid C++.
>>
>> Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
>> worrying about advanced C++ stuff on top as another patch. AFAICT, your
>> original patch is a strict improvement.
>
> Excuse me, where's the problem? The above example shows this
>
> @@ -105,8 +105,8 @@ char *url_decode(const char *url)
>          struct strbuf out = STRBUF_INIT;
> -       const char *slash = strchr(url, '/');
> +       const char *colon = strchr(url, ':');
> ...
>
> with current 4cb5d10b. This looks quite correct, no?

That's with the default heuristic; try something like this first to turn on userdiff:

	$ echo url.c diff=cpp >>.gitattributes
René
Junio C Hamano· Dec 6, 2011, 21:15 UTC · re: Thomas Rast · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

Thomas Rast <trast@student.ethz.ch> writes:
Show 7 quoted lines
> Actually (sadly) I'll have to revise it.  It doesn't match much of C++
> either, and I haven't yet come up with a reasonable regex that
> matches, say,
>
>   foo::Bar<int>::t& Baz::operator<<(
>
> which I would call ludicrous, but it's valid C++.

Heh, I'd rather not see us go that route, which would either end up implementing a C++ parser or reverting the heuristics back to "non-blank at the beginning of the line" that was already reasonably useful.

Thomas Rast· Dec 7, 2011, 08:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

Junio C Hamano wrote:
Show 13 quoted lines
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > Actually (sadly) I'll have to revise it.  It doesn't match much of C++
> > either, and I haven't yet come up with a reasonable regex that
> > matches, say,
> >
> >   foo::Bar<int>::t& Baz::operator<<(
> >
> > which I would call ludicrous, but it's valid C++.
> 
> Heh, I'd rather not see us go that route, which would either end up
> implementing a C++ parser or reverting the heuristics back to "non-blank
> at the beginning of the line" that was already reasonably useful.

Well, there are many things that we deliberately do not match right now and for which that's a good thing:

  label:
  public:
  void declaration_only(...);
  int global_variable;

At some point I was wondering whether it would be better to just declare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and otherwise match all '^[A-Za-z].*\(' but I may be missing something.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Johannes Sixt· Dec 7, 2011, 21:13 UTC · re: Thomas Rast · lore

Re: [PATCH] userdiff: allow * between cpp funcname words

Am 07.12.2011 09:04, schrieb Thomas Rast:
Show 26 quoted lines
> Junio C Hamano wrote:
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> Actually (sadly) I'll have to revise it.  It doesn't match much of C++
>>> either, and I haven't yet come up with a reasonable regex that
>>> matches, say,
>>>
>>>   foo::Bar<int>::t& Baz::operator<<(
>>>
>>> which I would call ludicrous, but it's valid C++.
>>
>> Heh, I'd rather not see us go that route, which would either end up
>> implementing a C++ parser or reverting the heuristics back to "non-blank
>> at the beginning of the line" that was already reasonably useful.
> 
> Well, there are many things that we deliberately do not match right
> now and for which that's a good thing:
> 
>   label:
>   public:
>   void declaration_only(...);
>   int global_variable;
> 
> At some point I was wondering whether it would be better to just
> declare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and
> otherwise match all '^[A-Za-z].*\(' but I may be missing something.

The current cpp pattern doesn't work that well with C++. Since it requires a blank before a name before the opening parentheses, it doesn't catch constructors:

Foo::Foo()

and it should fail for GNU style C function definitions as well (I didn't test):

void do_the_foo()

I'll run this pattern for a while:

diff.cpp.xfuncname=!^[a-zA-Z_][a-zA-Z_0-9]*[[:space:]]*:[[:space:]]*$ ^[a-zA-Z_].*

BTW, your match pattern requires an opening parenthesis; it would not catch class definitions.

-- Hannes

← back to recent threads