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

Various improvements to syntax highlighting. #917

Merged
merged 4 commits into from
Jun 29, 2016
Merged

Various improvements to syntax highlighting. #917

merged 4 commits into from
Jun 29, 2016

Conversation

datanoise
Copy link
Contributor

Look behind regexp patterns are not recommended in syntax files,
especially with the new NFA engine. So we should try to avoid them.
Highlighting of methods, fields, methods, structs and interfaces should
be much faster now.

Consolidated two options g:go_highlight_structs and
g:go_highlight_interfaces into one g:go_highlight_types.

Kent Sibilev added 4 commits June 29, 2016 00:05
Look behind regexp patterns are not recommended in syntax files,
especially with the new NFA engine. So we should try to avoid them.
Highlighting of methods, fields, methods, structs and interfaces should
be much faster now.

Consolidated two options g:go_highlight_structs and
g:go_highlight_interfaces into one g:go_highlight_types.
explicitly match variadic args, so they are not detected as goField
instead.
@@ -181,8 +181,7 @@ To change it:
let g:go_highlight_functions = 1
let g:go_highlight_methods = 1
let g:go_highlight_fields = 1
let g:go_highlight_structs = 1
let g:go_highlight_interfaces = 1
let g:go_highlight_types = 1
Copy link
Owner

Choose a reason for hiding this comment

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

This is a breaking change. Is there a way to not break this? I don't want people change their vimrc for no reason if possible :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that is quite unfortunate. I don't see an efficient way to separate parsing of type Name struct from type Name interface. Let me think about it a bit more. Maybe it can be done using syn-region syntax.

On the other hand, why do we need to distinguish these two cases?

Copy link
Owner

Choose a reason for hiding this comment

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

It was like that previously. We don't need to distinguish at all, it is just for backwards compatibility. But if you believe that making it backwards compatibility comes with increased complexity, I'm in favor of breaking it, so it means this change would be ok.

@fatih
Copy link
Owner

fatih commented Jun 29, 2016

Hi @datanoise. Thanks for this wonderful improvement. Is there a way you can split this into separate PR's for each highlight group? It's hard to follow what exactly it changes and you changed a lot of things :)

@datanoise
Copy link
Contributor Author

datanoise commented Jun 29, 2016

Just for the reference with these settings:

let g:go_highlight_functions = 1
let g:go_highlight_methods = 1
let g:go_highlight_fields = 0
let g:go_highlight_structs = 1
let g:go_highlight_interfaces = 1

I get the following syntax profiler report:

  TOTAL      COUNT  MATCH   SLOWEST     AVERAGE   NAME               PATTERN
  0.133099   527    21      0.001681    0.000253  goInterfaceDef     \(type\s\+\)\@<=\w\+\(\s\+interface\s\+{\)\@=
  0.131706   509    3       0.002651    0.000259  goStructDef        \(type\s\+\)\@<=\w\+\(\s\+struct\s\+{\)\@=
  0.129326   536    30      0.001473    0.000241  goFunction         \()\s\+\)\@<=\w\+\((\)\@=
  0.127595   526    20      0.001799    0.000243  goFunction         \(func\s\+\)\@<=\w\+\((\)\@=
  0.068593   520    14      0.000578    0.000132  goInterface        \(.\)\@<=\w\+\({\)\@=
  0.066483   509    3       0.000577    0.000131  goStruct           \(.\)\@<=\w\+\({\)\@=
  0.011295   531    27      0.000125    0.000021  goMethod           \(\.\)\@<=\w\+\((\)\@=
  0.003664   532    45      0.000066    0.000007  goBuiltins         \<\v(append|cap|close|complex|copy|delete|imag|len)\ze\(
  0.003609   508    2       0.000110    0.000007  goBuiltins         \<\v(make|new|panic|print|println|real|recover)\ze\(
  0.003355   506    0       0.000183    0.000007  goImaginaryFloat   \<-\=\.\d\+\%([Ee][-+]\=\d\+\)\=i\>

Same settings but with the patch:

  TOTAL      COUNT  MATCH   SLOWEST     AVERAGE   NAME               PATTERN
  0.004533   448    11      0.000061    0.000010  goTypeConstructor  \<\w\+{
  0.004355   439    2       0.000253    0.000010  goBuiltins         \<\v(make|new|panic|print|println|real|recover)\ze\(
  0.004079   451    33      0.000062    0.000009  goBuiltins         \<\v(append|cap|close|complex|copy|delete|imag|len)\ze\(
  0.003877   437    0       0.000084    0.000009  goImaginaryFloat   \<-\=\.\d\+\%([Ee][-+]\=\d\+\)\=i\>
  0.003741   437    0       0.000068    0.000009  goImaginaryFloat   \<-\=\d\+\.\d*\%([Ee][-+]\=\d\+\)\=i\>
  0.003735   437    0       0.000070    0.000009  goImaginary        \<-\=\d\+[Ee][-+]\=\d\+i\>
  0.003573   437    0       0.000051    0.000008  goFloat            \<-\=\.\d\+\%([Ee][-+]\=\d\+\)\=\>
  0.003555   437    0       0.000051    0.000008  goImaginary        \<-\=\d\+i\>
  0.003504   437    0       0.000054    0.000008  goHexadecimalInt   \<-\=0[xX]\x\+\>
  0.003501   437    0       0.000044    0.000008  goOctalInt         \<-\=0\o\+\>

I only had to replace

let g:go_highlight_structs = 1
let g:go_highlight_interfaces = 1

to

let g:go_highlight_types = 1

@fatih fatih merged commit 50ceca5 into fatih:master Jun 29, 2016
@fatih
Copy link
Owner

fatih commented Jun 29, 2016

Thanks a lot @datanoise I like the improvements. The syntax file really need some love (especially from someone with some regex fu skills). There are many things to do and I don't have the time to tackle all the issues.

@svanharmelen
Copy link
Contributor

@datanoise @fatih I noticed this PR changed some syntax highlighting among which this one:

image

Before the * would have been red and the received white... I guess this comes from the changes related to g:go_highlight_functions which now also syntax highlights the receiver type next to the function itself. As I did not set g:go_highlight_types I would expect this to not have changed.

Any way/settings I can tweak this so I get the desired behaviour?

@datanoise
Copy link
Contributor Author

I guess that makes sense. Could you please try this patch?

diff --git a/syntax/go.vim b/syntax/go.vim
index 9dcf004..1fb3f54 100644
--- a/syntax/go.vim
+++ b/syntax/go.vim
@@ -298,7 +298,6 @@ if g:go_highlight_functions != 0
 else
   syn keyword goDeclaration func
 endif
-hi def link     goReceiverType      Type
 hi def link     goFunction          Function

 " Methods;
@@ -320,6 +319,7 @@ if g:go_highlight_types != 0
   syn match goTypeDecl             /\<type\>/ nextgroup=goTypeName skipwhite skipnl
   syn match goTypeName             /\w\+/ contained nextgroup=goDeclType skipwhite skipnl
   syn match goDeclType             /\<interface\|struct\>/ contained skipwhite skipnl
+  hi def link     goReceiverType      Type
 else
   syn keyword goDeclType           struct interface
   syn keyword goDeclaration        type

@svanharmelen
Copy link
Contributor

svanharmelen commented Jul 5, 2016

@datanoise this patch stops highlighting the receiver, but it also (still) stops highlighting the pointer * sign... I assume the g:go_highlight_operators flag (which I have configured) should enable that part of the highlight?

@datanoise
Copy link
Contributor Author

That is quite more difficult to highlight. We cannot reuse the same goOperator group name to parse the pointer * sign. I had to introduce a new group goPointerOperator for that.

Could you please try this patch? If it works I'll send a pull request.

diff --git a/syntax/go.vim b/syntax/go.vim
index 9dcf004..be581b6 100644
--- a/syntax/go.vim
+++ b/syntax/go.vim
@@ -286,19 +286,22 @@ if g:go_highlight_operators != 0
   syn match goOperator /:=\|||\|<-\|++\|--/
   " match ...
   syn match goOperator /\.\.\./
+
+  hi def link     goPointerOperator   Operator
 endif
 hi def link     goOperator          Operator

 " Functions;
 if g:go_highlight_functions != 0
   syn match goDeclaration       /\<func\>/ nextgroup=goReceiver,goFunction skipwhite skipnl
-  syn match goReceiver /([^),]\+)/ contained nextgroup=goFunction contains=goReceiverType skipwhite skipnl
-  syn match goReceiverType /\(\s\|*\)\w\+)/hs=s+1,he=e-1 contained
+  syn match goReceiver /([^),]\+)/ contained nextgroup=goFunction contains=goReceiverVar skipwhite skipnl
+  syn match goReceiverVar /\w\+/ nextgroup=goPointerOperator,goReceiverType skipwhite skipnl contained
+  syn match goPointerOperator /\*/ nextgroup=goReceiverType contained skipwhite skipnl
+  syn match goReceiverType /\w\+/ contained
   syn match goFunction /\w\+/ contained
 else
   syn keyword goDeclaration func
 endif
-hi def link     goReceiverType      Type
 hi def link     goFunction          Function

 " Methods;
@@ -320,6 +323,7 @@ if g:go_highlight_types != 0
   syn match goTypeDecl             /\<type\>/ nextgroup=goTypeName skipwhite skipnl
   syn match goTypeName             /\w\+/ contained nextgroup=goDeclType skipwhite skipnl
   syn match goDeclType             /\<interface\|struct\>/ contained skipwhite skipnl
+  hi def link     goReceiverType      Type
 else
   syn keyword goDeclType           struct interface
   syn keyword goDeclaration        type

@svanharmelen
Copy link
Contributor

svanharmelen commented Jul 5, 2016

That seems to fix it, thx!

Unfortunately I found another issue:

image

In the line configure func([]byte) error, byte is not highlighted as a builtin type and error is highlighted as a function instead of a type 😞

@datanoise datanoise mentioned this pull request Jul 5, 2016
@datanoise
Copy link
Contributor Author

@svanharmelen thanks for the bug reports. I've included the fix to this issue to my pull request.

@svanharmelen
Copy link
Contributor

Thanks! 👍

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.

3 participants