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

build constraint fixes #589

Merged
merged 3 commits into from Nov 15, 2015
Merged

build constraint fixes #589

merged 3 commits into from Nov 15, 2015

Conversation

chrisnc
Copy link
Contributor

@chrisnc chrisnc commented Nov 1, 2015

Some improvements and fixes to the g:go_highlight_build_constraints feature:

  • add missing GOARCH values: arm64, ppc64, and ppc64le
  • remove the multiline comment pattern (build constraints must always be line comments)
  • add a required trailing space to the build constraint region start pattern to avoid highlighting things like // +buildwindows which is not a valid build constraint
  • only highlight the leading // as a comment; other words in the build constraint can be valid build tags other than GOOS, GOARCH, cgo, and ignore, so they should be treated as identifiers
  • add a syntax match for package documentation comments that supersedes the build constraint highlighting; with this, you know when you've accidentally written a build constraint that will be ignored

This won't highlight the constraint because it's followed by package:

// +build linux
package foo

This will highlight the constraint:

// +build linux

package foo

The first commit just fixes some inconsistent whitespace in syntax/go.vim.

I appreciate any feedback or suggestions. Thanks!

@fatih
Copy link
Owner

fatih commented Nov 1, 2015

Thanks @chrisnc This is huge and awesome!

I'll be not available for two weeks. I'll be on my way to dotgo.eu for my talk, so I have little time. Just wanted to you give a heads up here.

add missing architectures

remove multiline comment pattern for build constraints (build
constraints must always be line comments)

add a required trailing space to the build constraint region start pattern
this avoids highlighting, e.g., // +buildwindows

add a syntax match for package documentation comments that disables
highlighting build constraints when they will be interpreted as package
documentation instead

in build constraints, only use the comment color to highlight the
leading //
@chrisnc
Copy link
Contributor Author

chrisnc commented Nov 1, 2015

No problem. Let me know when you get a chance to try it out. I've really appreciated the plugin so far, so I'm glad to contribute to it.

I have some other ideas for doing similar kinds of highlighting assistance for //export and cgo. This part was a good start because it's a feature that's already there.

@chrisnc
Copy link
Contributor Author

chrisnc commented Nov 1, 2015

I'm not sure quite how to do this, but I'd like to also prevent highlighting build constraints that appear after the package, as these are also ignored.

@fatih
Copy link
Owner

fatih commented Nov 15, 2015

Hi @chrisnc. Just tested it and this is really awesome :) Works perfect and I really like it. Awesome contribution 👍

fatih added a commit that referenced this pull request Nov 15, 2015
@fatih fatih merged commit 69cf03d into fatih:master Nov 15, 2015
@chrisnc
Copy link
Contributor Author

chrisnc commented Nov 15, 2015

Great! Thanks for merging.

@chrisnc
Copy link
Contributor Author

chrisnc commented Nov 15, 2015

What is your feeling on leaving this feature off by default? I don't feel strongly either way, but I wonder if it might save some people who didn't know about the // +build followed by package issue and also haven't read the README or docs to know about the option (this was me for several months).

@chrisnc chrisnc deleted the syntax branch November 15, 2015 20:31
@fatih
Copy link
Owner

fatih commented Nov 15, 2015

@chrisnc syntax highlighting is expensive. Most of the complaints comes from vim-go being slow and when profiled %99 is syntax highlighting. That's the reason we disable most of the syntax highlighting functions. Another reason is that people don't like colorful code. I know this is really useful, but in the end what matters is the general speed for all of our users.

@chrisnc
Copy link
Contributor Author

chrisnc commented Nov 15, 2015

Ah, okay, that makes sense. I wasn't thinking about the perf impact.

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

2 participants