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

Add operator symbols in regex to show correct field highlight termination #1907

Merged
merged 1 commit into from
Jan 13, 2019

Conversation

zkry
Copy link
Contributor

@zkry zkry commented Aug 6, 2018

Fixes #1849. This is indeed a problem even when gofmt is on (ex. inside array index brackets). This should cover all of the operators.

@fatih
Copy link
Owner

fatih commented Aug 6, 2018

Hi @zkry

For this kind of PR's a before/after screenshot on what this is fixed is appreciated.

@zkry
Copy link
Contributor Author

zkry commented Aug 6, 2018

Oh, ok. Here is a screenshot showing a simple example of before and after the change.

ekran resmi 2018-08-06 23 46 47

@arp242
Copy link
Contributor

arp242 commented Aug 6, 2018

Also, could you post the results from the benchmarks with ./scripts/bench-syntax? Then we have an idea how this will affect performance (it's already quite slow). Thanks!

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 25, 2018

@Carpetsmoker I benchmarked these changes against master; they end up slowing things down about 10%, but if this PR goes one step further and uses \%( where possible, then this PR and the use of \%( results in a zero net change in performance.

@zkry
Copy link
Contributor Author

zkry commented Aug 27, 2018

@bhcleek Thanks. I updated the PR with your suggestion. I was having trouble figuring out how to run the benchmark and get the relevant info. I'll write here when I can confirm that there is now slowdown.

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 27, 2018

@zkry you'll likely need to replace more \( with \%( than just that one place; when I tested there were a several places that I updated to use \%(....

syntax/go.vim Outdated
@@ -291,7 +291,7 @@ hi def link goFunctionCall Type

" Fields;
if go#config#HighlightFields()
syn match goField /\.\w\+\([.\ \n\r\:\)\[,]\)\@=/hs=s+1
syn match goField /\.\w\+\%([}\!=\><\^|&%\/*\-\+.\ \n\r\:\)\[\],]\)\@=/hs=s+1
Copy link
Contributor

Choose a reason for hiding this comment

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

This regular expression is now very hard to understand and/or maintain. It should probably documented better and split over multiple lines.

For example:

vim-go/syntax/go.vim

Lines 108 to 120 in b81a447

" [n] notation is valid for specifying explicit argument indexes
" 1. Match a literal % not preceded by a %.
" 2. Match any number of -, #, 0, space, or +
" 3. Match * or [n]* or any number or nothing before a .
" 4. Match * or [n]* or any number or nothing after a .
" 5. Match [n] or nothing before a verb
" 6. Match a formatting verb
syn match goFormatSpecifier /\
\([^%]\(%%\)*\)\
\@<=%[-#0 +]*\
\%(\%(\%(\[\d\+\]\)\=\*\)\|\d\+\)\=\
\%(\.\%(\%(\%(\[\d\+\]\)\=\*\)\|\d\+\)\=\)\=\
\%(\[\d\+\]\)\=[vTtbcdoqxXUeEfFgGsp]/ contained containedin=goString,goRawString

(which is still not great, but better than this).

It may also be worth adding \v at the start of this pattern for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I can clean this regex along with updating \( with \%(. Sorry for having stepped away from this issue for such a long time by the way.

In addition, change sub-expression regexs \( to \%( to speed up syntax
time.
@zkry
Copy link
Contributor Author

zkry commented Oct 20, 2018

I noticed how were weren't using the \1 - \9 feature of regexes at all so it seemed safe to update all instances of \( to \%(. It was a good idea to clean up the regex too. It would always take me forever to understand what it was doing.

@bhcleek bhcleek added this to the vim-go 1.20 milestone Nov 4, 2018
bhcleek added a commit that referenced this pull request Jan 13, 2019
@bhcleek bhcleek merged commit 0a621c5 into fatih:master Jan 13, 2019
bhcleek added a commit that referenced this pull request Jan 13, 2019
PR #1907 was previously merged by doing a rebase, resolving conflicts,
and merging the result. This merge is done using the `ours` strategy to
keep the history clean and make it clear that the PR was actually
merged.
@bhcleek
Copy link
Collaborator

bhcleek commented Jan 13, 2019

Thanks, @zkry !

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

4 participants