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

Sync our ctags to the latest tag version (p6.1.20240421.0) #3859

Merged
merged 16 commits into from
May 14, 2024

Conversation

techee
Copy link
Member

@techee techee commented Apr 27, 2024

There were mostly expected "problems" like new source files that had to be added, rename of perl6.c to raku.c, update of kind mappings, etc.

The only unexpected thing is the behavior of the javascript parser - this is from the javascript commit message:

There are lots of differences because of

universal-ctags/ctags@6d85089

Also

universal-ctags/ctags@b1870b8

seems to confuse the parser in simple.js so it doesn't generate my_global_var2.

Finally, Geany reports

(geany:820768): Tagmanager-WARNING **: 20:38:28.755: ignoring null tag in /home/parallels/projects/geany/doc/reference/jquery.js(line: 2, language: JavaScript)

@b4n Does the PR in general and the javascript parser in particular look good to you? I didn't investigate the javascript parser differences much as I'm not a javascript user myself. At least the NULL warning should be fixed though.

The file name has been changed and we cannot use the script for the
update.
Geany compiles fine after this step, it just doesn't run because
of incorrect mappings.
The parser isn't used by Geany itself but is started from within
the cxx parser and the asm parser which don't run correctly
without it.
All extra kinds are mapped to undef_t.

Geany starts after this step.
Without it we might not get proper hierarchy in the sidebar because
of missing parent tags.
Note: in C and other languages function arguments contain braces
but it doesn't seem to be the case here. So instead of mymacro(args)
we get mymacroargs in the pretty-printer script. Not sure if it's
our problem or if the asm parser should be updated upstream.
When role kinds are enabled, they report tags also for checks like

#ifdef MY_MACRO

in which we are not interested.
There are lots of differences because of

universal-ctags/ctags@6d85089

Also

universal-ctags/ctags@b1870b8

seems to confuse the parser in simple.js so it doesn't generate
my_global_var2.

Finally, Geany reports

(geany:820768): Tagmanager-WARNING **: 20:38:28.755: ignoring null tag in /home/parallels/projects/geany/doc/reference/jquery.js(line: 2, language: JavaScript)
Even though the output isn't perfect, I'd prefer not to maintain
our own version of the parser (with different kinds which makes
ctags files incompatible with our version).
@techee
Copy link
Member Author

techee commented Apr 27, 2024

I also added a commit updating the matlab parser to the upstream regex-based one. I'd like to avoid the situation that we have to maintain our (non-regex) version of the parser like in:

#3358
#3563

Also, the kinds generated by the upstream parser are different than those we parse using our parser so the ctags file are incompatible.

@b4n
Copy link
Member

b4n commented Apr 27, 2024

I'll look into this Soon™, but the JS changes might have an issue (might be what you see, might be something else), see this which doesn't look right to me.

In any case, thanks for doing this :)

@b4n
Copy link
Member

b4n commented May 10, 2024

Regarding JS:

The only unexpected thing is the behavior of the javascript parser - this is from the javascript commit message:

There are lots of differences because of

universal-ctags/ctags@6d85089

Yeah, all this look OK. Some can be viewed as implementor's choice, but it's fair so I'm fine with it.

Also

universal-ctags/ctags@b1870b8

seems to confuse the parser in simple.js so it doesn't generate my_global_var2.

Despite what's in the test file, this is not valid JS (AFAICT, or as far as SpiderMonkey tells me), so it's understandable the parser doesn't get along very well with it. If you add a semicolon after it it fixes it, as it then consider it the end of the construct and "resets" itself.

Finally, Geany reports

(geany:820768): Tagmanager-WARNING **: 20:38:28.755: ignoring null tag in /home/parallels/projects/geany/doc/reference/jquery.js(line: 2, language: JavaScript)

Is this actually anything new? I just fixed a few upstream in universal-ctags/ctags#3993, but I believe we already had this before.

Also, there's one instance in that jQuery code where it's somewhat expected, as there is indeed a zero-length property. For that remaining one, I'm not sure what the parser should do, as it can't really emit an empty name (format would probably not accept it, and anyway consumers are unlikely to like it), yet there is one in the input…

@b4n Does the PR in general and the javascript parser in particular look good to you? I didn't investigate the javascript parser differences much as I'm not a javascript user myself. At least the NULL warning should be fixed though.

Yeah, the JS changes look (almost) fine. I raised a couple issues upstream, but it's nothing serious nor that actually affect us much (see e.g. universal-ctags/ctags#3995 and universal-ctags/ctags#3997).

Two things:

  • we should probabmy use a newer snapshot (some parser changes, e.g. related to class vs. object vs. variable are gonna lead to additional test result changes, which are admittedly better in the newer version, and I'm not sure it's worth having an intermediary step here)
  • we should probably rather fix simple.js test to remove the invalid syntax that confuses the parser. I'll try and do that Soon™.

@b4n
Copy link
Member

b4n commented May 10, 2024

BTW:

Sync our ctags to the latest tag version (p6.1.20240421.0)

Tags p* are NOT releases, but probably pre-versions, or even nightly snapshots. Releases are v*.
It's probably fine, but noteworthy 🙂

@techee
Copy link
Member Author

techee commented May 10, 2024

@b4n Thanks for having a look at the javascript parser.

we should probabmy use a newer snapshot (some parser changes, e.g. related to class vs. object vs. variable are gonna lead to additional test result changes, which are admittedly better in the newer version, and I'm not sure it's worth having an intermediary step here)

Can do that. Should it be part of this PR or a separate one after this one is merged?

we should probably rather fix simple.js test to remove the invalid syntax that confuses the parser. I'll try and do that Soon™.

Could do it myself but better someone who at least remotely knows what the correct syntax is :-).

Tags p* are NOT releases, but probably pre-versions, or even nightly snapshots. Releases are v*.
It's probably fine, but noteworthy 🙂

OK, I didn't notice the release tags. But since these are just about once a year, I guess we want the latest thing and "risk" the consequences, right?

src/tagmanager/tm_parser.c Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a bit sad… I'll see if I can get those fixed upstream (it's easy enough to patch actually -- yet, I don't have a clue about matlab 😁 )

src/tagmanager/tm_parser.c Outdated Show resolved Hide resolved
@b4n
Copy link
Member

b4n commented May 10, 2024

Can do that. Should it be part of this PR or a separate one after this one is merged?

Whichever you prefer. I'd have said this one given it'll introduce yet another set of changes that are probably best diffed from current master, but I don't really mind either way in practice, so if it's easier for you just do another one afterwards.

Tags p* are NOT releases, but probably pre-versions, or even nightly snapshots. Releases are v*.
It's probably fine, but noteworthy 🙂

OK, I didn't notice the release tags. But since these are just about once a year, I guess we want the latest thing and "risk" the consequences, right?

Yeah, I think we can "risk it" 🙂

@techee
Copy link
Member Author

techee commented May 10, 2024

Can do that. Should it be part of this PR or a separate one after this one is merged?

Whichever you prefer. I'd have said this one given it'll introduce yet another set of changes that are probably best diffed from current master, but I don't really mind either way in practice, so if it's easier for you just do another one afterwards.

I just wanted to avoid rebasing this PR on top of another ctags version so I'd prefer a separate PR (which will be much smaller and easier to review).

b4n added 2 commits May 13, 2024 22:20
Remove invalid code (despite what the code says) that now confuses the
parser, leading to missing tags after it.
@b4n
Copy link
Member

b4n commented May 13, 2024

@techee I just added 2 more commits -- feel free to comment and reject them :)
Anyway, looks good to me now.

Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't review all parser's changes (sorry 😭), but the Geany changes, tests and behavior look good 👍

@techee techee merged commit 2d1b3d5 into geany:master May 14, 2024
7 checks passed
@techee
Copy link
Member Author

techee commented May 14, 2024

@techee I just added 2 more commits -- feel free to comment and reject them :)

Yes, that's what I'm gonna do, just to enjoy the power :-).

Anyway, great, I've merged this PR. If you plan to do some more ctags changes, just ping me when you are done and the changes are merged upstream and I'll prepare another sync PR with them.

@b4n
Copy link
Member

b4n commented May 16, 2024

@techee FWIW, the PRs I wanted merged now are, so it's whenever you like :)

@techee
Copy link
Member Author

techee commented May 16, 2024

Great, I think I'll just wait for the next ctags tag and will use that one.

@b4n b4n added this to the 2.1 milestone Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants