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.

Sign up for GitHub

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

Jump to bottom

Default to cc-mode-get-declarations #97

Merged
merged 4 commits into from
Jan 23, 2023
Merged

Default to cc-mode-get-declarations #97

merged 4 commits into from
Jan 23, 2023

Conversation

Hi-Angel
Copy link
Collaborator

It isn't actually specific to cc-mode, and works in a way similar to the previous default scan-identifiers, except that because it only goes through places with properties set, it is:

  1. more performant due to less motion
  2. less likely to introduce wrong coloring as in js2-mode: Comments are colorized #40 or Every single word in a JSX block is colored in js2-jsx-mode #62

We also rename the function to remove infix cc-mode as it isn't (and never really have been) specific to c-mode.

Fixes: #94

@Hi-Angel
Copy link
Collaborator Author

From my experiments everything seems fine, but more testing would be appreciated!

@Hi-Angel
Move "C/C++ mode support" code to the top of the paragraph
5fe20c3 
This is a preparation before the next commit where that part will no
longer be c-mode specific.
@Hi-Angel
Default to cc-mode-get-declarations
9e4d1cc 
It isn't actually specific to cc-mode, and works in a way similar to the
previous default `scan-identifiers`, except that because it only goes
through places with properties set, it is:

1. more performant due to less motion
2. less likely to introduce wrong coloring as in ankurdave#40 or ankurdave#62

We also rename the function to remove infix `cc-mode` as it isn't (and
never really have been) specific to c-mode.

Fixes: ankurdave#94
@Hi-Angel
Copy link
Collaborator Author

Hi-Angel commented Jan 22, 2023 •
edited
Loading

FTR: after this PR is merged, I think further simplifications to scan-identifiers can be done. I don't think it should touch regions we already fontified.

I'll refrain from doing that as part of this PR though because that would change the way scan-identifiers moves, and I'm not sure it has good motion algorithm right now, so something may break. Specifically, sometimes it moves explicitly to the next point with faces changes, however it shouldn't be really concerned with those at all as identifier-usages may or may not have faces set.

@Hi-Angel
Copy link
Collaborator Author

FTR: after this PR is merged, I think further simplifications to scan-identifiers can be done. I don't think it should touch regions we already fontified.

Done. After further research I figured it may be skipped with minimal changes to the motion. Tested with c-mode, python-mode, js-mode — see no problems.

As a side note, I realized scan-identifiers does a bit more, so can't make it ignore face-change regions completely. That's because the get-declaration functions doesn't do any colorization, the scan-identifiers does. Not sure if changing that will improve performance or anything; may affect backward compatibility OTOH. So probably gonna leave that as is.

@Hi-Angel
Copy link
Collaborator Author

Hi-Angel commented Jan 22, 2023 •
edited
Loading

UPD: further simplification to last commit: I just realized that next-property-change actually goes not necessarily to the next property, but also to a text that has no property on it. So simply removing the comparison with color-identifiers:fontified should be fine. That changes the behavior in such a way, so upon encountering a color-identifiers:fontified region we immediately go the end of it and try again.

@Hi-Angel Hi-Angel force-pushed the default-to-cc-mode branch 2 times, most recently from e6226ee to 3a9e24a Compare January 22, 2023 18:23
@Hi-Angel
Copy link
Collaborator Author

So, let's merge this and #98 I think. I have used them for a day with C/Meson/Python, no problems, everything seems fine.

@Hi-Angel Hi-Angel merged commit 307fabe into ankurdave:master Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Defaulting to cc-mode-get-declarations instead of scan-identifiers
1 participant
@Hi-Angel