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

Pascal: Use separate kind for type identifiers #2360

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ntrel
Copy link
Member

@ntrel ntrel commented Oct 15, 2019

Geany has implemented reading an identifier straight after the type keyword. Universal ctags doesn't have this. This pull uses a separate tag kind, instead of (ab)using the function kind.

May be relevant to #2358 (which I can't reproduce on Windows).

@ntrel
Copy link
Member Author

ntrel commented Oct 15, 2019

I need to update bug612019.pas.tags.

@elextr
Copy link
Member

elextr commented Oct 15, 2019

@ntrel you need to find why the Travis tests pass on windows and fail on Linux, and that might help #2358 that you can't reproduce on Windows.

Also you need to push upstream, otherwise its likely to get overwritten by the next update from ctags.

@codebrainz
Copy link
Member

Also you need to push upstream, otherwise its likely to get overwritten by the next update from ctags.

If the parser has already diverged from upstream as suggested in the (edited) description, then while it would be nice to sync all the changes back upstream, it probably shouldn't be a blocker for this PR to do it.

@elextr
Copy link
Member

elextr commented Oct 16, 2019

Well, if local changes are not pushed upstream it will never get synced, and I said nothing about it being a blocker for this.

@codebrainz
Copy link
Member

You said @ntrel needs to push upstream, but it sounds like it was already diverged, so IMO he doesn't have to do it (inferred "for this PR"), just that ideally "someone" would do it eventually.

@elextr
Copy link
Member

elextr commented Oct 16, 2019

(inferred "for this PR")

Any inference that I said this PR should depend on upstream accepting the change is incorrect, I didn't say that at all.

This PR is to fix a crash (see #2358) so a short term local patch is acceptable, but if changes are not pushed upstream then Geany will permanently diverge again.

@codebrainz
Copy link
Member

Any inference that I said this PR should depend on upstream accepting the change is incorrect, I didn't say that at all.

Not just whether or not they accept the changes, the act of someone going through the forked parser and upstream parser and manually syncing any unrelated changes is out of scope for this PR, IMO.

but if changes are not pushed upstream then Geany will permanently diverge again.

The description mentions that they've already diverged, so the Pascal parser was already out of sync and permanently diverged (until someone syncs it).

@elextr
Copy link
Member

elextr commented Oct 16, 2019

so the Pascal parser was already out of sync and permanently diverged (until someone syncs it).

And thats why it needs some annoying guy to keep mentioning it :)

@ntrel
Copy link
Member Author

ntrel commented Oct 16, 2019

This PR is to fix a crash

Not really, it's possible it does but that wasn't the main reason.

if changes are not pushed upstream then Geany will permanently diverge again

Parsing the first identifier after type is already a divergence. ATM I'm not confident of its correct parsing, this pull just makes geany see the tag differently.

@ntrel ntrel marked this pull request as ready for review October 16, 2019 16:08
@ntrel
Copy link
Member Author

ntrel commented Oct 16, 2019

Updated tests, don't set an arglist & vartype for type tags, fix parsing comment before identifier -
aae0a21. That commit may fix #2358, perhaps along with the defensive nulling of
c46d2df.

@ntrel
Copy link
Member Author

ntrel commented Oct 16, 2019

I've made universal-ctags/ctags#2241 but that needs ctags tests.

@elextr
Copy link
Member

elextr commented Oct 16, 2019

@ntrel, travis seems to like it, what about adding the problem from #2358 to bug612019.pas or making a new test to test it?

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

Successfully merging this pull request may close these issues.

None yet

3 participants