Hi René,
Thanks for feedback. I am already sleeping but let me try to reply anyway,
even if I don't really understand you.
On 09/11, René Scharfe wrote:
>
> Am 11.09.23 um 14:12 schrieb Oleg Nesterov:
> > show_funcname_line() returns when "lno <= opt->last_shown" and this
> > is not right in that the ->last_shown line (which matched the pattern)
> > can also have the actual function name we need to report.
> >
> > Change this code to check "lno < opt->last_shown". While at it, move
> > this check up to avoid the unnecessary "find the previous bol" loop.
> >
> > Note that --lno can't underflow, lno==0 is not possible in this loop.
> >
> > Simple test-case:
> >
> > $ cat TEST.c
> > void func(void);
> >
> > void func1(xxx)
> > {
> > use1(xxx);
> > }
> >
> > void func2(xxx)
> > {
> > use2(xxx);
> > }
> >
> > $ git grep --untracked -pn xxx TEST.c
> >
> > before the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > after the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c=3=void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c=8=void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > which looks much better to me.
>
> Interesting idea to treat function lines as annotations of matches
> instead of as special context.
Sorry, I don't understand... Let me repeat I am not familiar with this
code and terminology. Could you spell please?
> Showing lines twice feels wasteful, but
> at least for -p it might be justifiable from that angle.
Just in case... say, "func1" is reported twice only when it is really
needed. From the "after the patch" output above:
TEST.c:3:void func1(xxx)
this is what we already have without this patch
TEST.c=3=void func1(xxx)
this is what we have with this patch because the next
TEST.c:5: use1(xxx);
line needs the proper funcname line, and without this patch it would be
"void func()" which has nothing to do with use1(xxx),
If I do, say,
./git grep --untracked -pn func1 TEST.c
then (with or without this patch) the output is
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
in this case there is no reason to report "=void func1(xxx)".
> Wouldn't you
> have to repeat function line 3 before the match in line 8, though?
Why?
> The reason for not repeating a matched function line was that it
> doesn't add much information under the assumption that it's easy to
> identify function lines visually.
But it is not. Lets look again at the "before the patch:" output above,
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
TEST.c:5: use1(xxx);
TEST.c:8:void func2(xxx)
TEST.c:10: use2(xxx);
it looks as if every "xxx" match is inside the (unrelated) func().
OK, "visually" you can also notice the "void funcX(xxx)" declarations
and understand whats going on.
But a) I don't think this is always easy, and b) it is certainly not
easy when you use "git-grep -p" in scripts. Please see 0/1.
> The patch would need to update Documentation/git-grep.txt as well to
> reflect the changed output.
Hmm... From Documentation/git-grep.txt:
-p::
--show-function::
Show the preceding line that contains the function name of
the match, unless the matching line is a function name itself.
...
this is still true after this patch. How do you think I should update this
section?
Oleg.
