Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

failed to parse filename #28

Closed
kiani45 opened this issue Jul 11, 2021 · 14 comments
Closed

failed to parse filename #28

kiani45 opened this issue Jul 11, 2021 · 14 comments

Comments

@kiani45
Copy link

kiani45 commented Jul 11, 2021

Looks like a regression of commit [b9bd846]:(b9bd846)

I use "emacs -nw" with gtags as cscope backend.
My cpp source file has :

...
// kkk" before void WtRaw::Convert()
#define ConvertFpN(...) 0  // kkk
...

When I try to search text "kkk", I got:
image

While if I revert commit b9bd84, I got correct output:
image

@dkogan
Copy link
Owner

dkogan commented Jul 11, 2021

Thanks for the report. This certainly looks like a problem. @bertogg: this was your patch; can you please take a look? I'll try to get to it within the next week or two otherwise.

@bertogg
Copy link
Contributor

bertogg commented Jul 11, 2021

Can you provide the complete source file?

Also, what command do you use to find that string? M-x cscope-find-this-text-string ?

@kiani45
Copy link
Author

kiani45 commented Jul 12, 2021

Here's the source file (renamed file extension as .txt) and I use cscope-find-egrep-pattern SYMBOL where SYMBOL="kkk"

@bertogg
Copy link
Contributor

bertogg commented Jul 12, 2021

This is what I get with your example after searching kkk with M-x cscope-find-egrep-pattern:

*** wt_data.cpp:
<unknown>[282]                 // kkk
<unknown>[283]                 #define ConvertFpN(...)  // kkk

Reverting the commit that you mention does not seem to make any difference here.

This seems like the correct behavior to me. I guess your problem is in cscope itself, I'm using version 15.9-1, from Debian.

@dkogan
Copy link
Owner

dkogan commented Jul 12, 2021

@bertogg: thanks for looking at it. I just looked on my end, and I see the same thing: it works ok with and without the patch with the latest cscope in Debian. I also see that I completely forgot to update the Debian package after your patch. Sorry about that. I'll push a new version when the freeze is done.

@kiani45: we need more information to reproduce your problem. If you can run more experiments, that'd be nice.

@bertogg
Copy link
Contributor

bertogg commented Jul 12, 2021

Oh, it would actually be nice to have this fixed for bullseye, but I guess it's a bit late for that? Thanks anyway

@dkogan
Copy link
Owner

dkogan commented Jul 12, 2021 via email

@kiani45
Copy link
Author

kiani45 commented Jul 12, 2021

It's kind of weird.

I guess my cscope should be fine:
image
image

But I still has same problem with xcscope.el even when switch to cscope as backend (I used global-cscope before):
image

My emacs version is 26.3
Is there any further information you need?

@kiani45
Copy link
Author

kiani45 commented Jul 12, 2021

I have simplify the reproduce environment:
simple .emacs:

(package-initialize)
(require 'xcscope)

My xscope.el (from Elpa):
.emacs.d/elpa/xcscope-20201025.2002/xcscope.el
(I have tried replacing with the latest xcsope.el but still have same issue)

I have attached my cscope.out & wt_data.cc
out.gz

@dkogan
Copy link
Owner

dkogan commented Jul 19, 2021

Hi. Thanks for providing that tarball. First, let's make sure that cscope itself is doing what it should. I'd be surprised if it isn't, but let's check just in case. I'm seeing this:

dima@shorty:/tmp/xxx$ cscope -f cscope.out -L -6 kkk
wt_data.cc <unknown> 282 // kkk" before void WtRaw::Convert()
wt_data.cc <unknown> 283 #define ConvertFpN(...) 0  // kkk

dima@shorty:/tmp/xxx$ cscope -f cscope.out -L -6 kkk | md5sum
908026e542c573bd7cf463a440d6d056  -

This is with the two files in your archive. Can you confirm that this is what you get, including the md5 sum? If so, I'm going to have another question for you, but let me look at the code first

@dkogan dkogan closed this as completed in d228d75 Jul 19, 2021
@dkogan
Copy link
Owner

dkogan commented Jul 19, 2021

OK, never mind. I see the problem. And I think I just fixed it.

@kiani45: can you please try the commit I just pushed? It should fix the problem. And thanks for reporting the bug.

@bertogg: you changed the regex parsing the cscope output to be able to accept whitespace in filenames. This is ambiguously-defined, however, so in @kiani45's case, the filename is extracted incorrectly: whitespace in non-filename text is interpreted as being a part of the filename. I just reverted the regex change in your patch to fix THIS bug (since I bet this actually breaks many usages). But that revert crippled your feature. Can you think of a more reliable way to parse out the filenames? I'd love to merge something that solves your problem without adverse effects. A bit of elisp that demonstrates the issue:

(let ((lines '("wt_data.cc <unknown> 282 // kkk\" before void WtRaw::Convert()\n"
               "wt_data.cc <unknown> 283 #define ConvertFpN(...) 0  // kkk\n")))

  (mapcar (lambda (line)
            (and (string-match
                  "^\\([^\t]+\\)[ \t]+\\([^ \t]+\\)[ \t]+\\([0-9]+\\)[ \t]+\\(.*\\)\n"
                  line)
                 (substring line (match-beginning 1) (match-end 1))))
          lines))

This applies the regex (including your modification) to the lines of data @kiani45 was producing. It returns the filename, as extracted by the regex. If you evaluate that form, you'll see that it's extracting the filename correctly for the first cscope result, but not so for the second result. Thanks!

The reason my earlier testing didn't suggest that anything was wrong is that I was running the old code by accident in both cases.

@dkogan
Copy link
Owner

dkogan commented Jul 19, 2021

The reason the input here caused an issue is because of the extra numerical token in the input: the "0". We can adjust the regex to match the filename minimally, which would fix the problem for this input, but then you won't be able to have filenames "AAA BBB NUMBER CCC". I'm thinking that's the best we can do here. Thoughts?

@kiani45
Copy link
Author

kiani45 commented Jul 19, 2021

@dkogan
Just tried the latest one and it has fixed the issue.
Thanks a lot!

@bertogg
Copy link
Contributor

bertogg commented Nov 22, 2021

The reason the input here caused an issue is because of the extra numerical token in the input: the "0". We can adjust the regex to match the filename minimally, which would fix the problem for this input, but then you won't be able to have filenames "AAA BBB NUMBER CCC". I'm thinking that's the best we can do here. Thoughts?

What would happen if the filename contains numbers but not spaces? If that works then I'm fine with the solution.

I think filenames with numbers are way more common than filenames with spaces, so in doubt I would support the former.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants