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

Update keyword.operator.comparison.go regex #145

Merged
merged 2 commits into from Dec 4, 2017

Conversation

Projects
None yet
2 participants
@lzambarda
Contributor

lzambarda commented Nov 30, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

The regex to match keyword.operator.comparison.go
(==|!=|<=|>=|<[^<]|>[^>])
contains a mistake introduced to avoid matching the bitshift operators. [^<] and [^>] will match anything following the first less/greater than symbol (this highlights <3 and >3 and is even more obvious with <3.0 or >2.0
The correct regex is
(==|!=|<=|>=|<(?!<)|>(?!>))
as it uses negative lookaheads to avoid highlighting characters immediately following the less/greater than symbol.

Alternate Designs

If the negative lookahead is considered to be too greedy, the regex catching the bitwise operators could be placed before this regex and thus allowing to remove [^<] and [^>] from this regex.

Benefits

Resolving several minor erroneous syntax highlights involving the less than/greater than operators.

Possible Drawbacks

No drawbacks identified so far.

Applicable Issues

Syntax highlights of anything in the regex format of:
<[a-zA-Z0-9]
e.g. <3 >2.5 <i and more with this syntax

Update keyword.operator.comparison.go regex
The regex to match keyword.operator.comparison.go
(==|!=|<=|>=|<[^<]|>[^>])
contains a mistake introduced to avoid matching the bitshift operators. [^<] and [^>] will match anything following the first less/greater than symbol (this highlights <3 and >3 and is even more obvious with <3.0 or >2.0 
The correct regex is
(==|!=|<=|>=|<(?!<)|>(?!>))
as it uses negative lookaheads to avoid highlighting characters immediately following the less/greater than symbol.
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Dec 3, 2017

Member

Excellent catch @TheHominid.

If the negative lookahead is considered to be too greedy, the regex catching the bitwise operators could be placed before this regex and thus allowing to remove [^<] and [^>] from this regex.

I think I'd prefer this solution. Could you also add some new specs to ensure that this behavior doesn't regress in the future?

Member

50Wliu commented Dec 3, 2017

Excellent catch @TheHominid.

If the negative lookahead is considered to be too greedy, the regex catching the bitwise operators could be placed before this regex and thus allowing to remove [^<] and [^>] from this regex.

I think I'd prefer this solution. Could you also add some new specs to ensure that this behavior doesn't regress in the future?

@lzambarda

This comment has been minimized.

Show comment
Hide comment
@lzambarda

lzambarda Dec 4, 2017

Contributor

Hi,
Thanks, I've spent some time try rearranging the regex in grammars/go.cson and it turned out to be quite tricky. There's a comment about these regex:

'comment': 'Note that the order here is very important!'

And indeed the only way to bot break everything is to split some of these regex to then create the proper order of execution. Now, this completely defeats the purpose of avoiding negative lookaheads for potential performance issues.

So I've spent some additional time investigating whether the negative lookahead is indeed resource hungry and it doesn't seem so, also because it is combined with a fixed character that is checked before and thus the overall complexity is significantly reduced. (Sources:
https://blogs.msdn.microsoft.com/bclteam/2010/08/03/optimizing-regular-expression-performance-part-ii-taking-charge-of-backtracking-ron-petrusha/
https://stackoverflow.com/questions/35476547/regex-negative-lookbehind-and-lookahead-equivalence-and-performance)

As solution I just left unchanged my previous fix and I've added in the specs in spec/go-spec.coffee to test that the fix works (I've appended this after the various comparisons tests so that I'm sure I'm not breaking anything).

Contributor

lzambarda commented Dec 4, 2017

Hi,
Thanks, I've spent some time try rearranging the regex in grammars/go.cson and it turned out to be quite tricky. There's a comment about these regex:

'comment': 'Note that the order here is very important!'

And indeed the only way to bot break everything is to split some of these regex to then create the proper order of execution. Now, this completely defeats the purpose of avoiding negative lookaheads for potential performance issues.

So I've spent some additional time investigating whether the negative lookahead is indeed resource hungry and it doesn't seem so, also because it is combined with a fixed character that is checked before and thus the overall complexity is significantly reduced. (Sources:
https://blogs.msdn.microsoft.com/bclteam/2010/08/03/optimizing-regular-expression-performance-part-ii-taking-charge-of-backtracking-ron-petrusha/
https://stackoverflow.com/questions/35476547/regex-negative-lookbehind-and-lookahead-equivalence-and-performance)

As solution I just left unchanged my previous fix and I've added in the specs in spec/go-spec.coffee to test that the fix works (I've appended this after the various comparisons tests so that I'm sure I'm not breaking anything).

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Dec 4, 2017

Member

Seems fine to me. Thanks for the PR!

Member

50Wliu commented Dec 4, 2017

Seems fine to me. Thanks for the PR!

@50Wliu 50Wliu merged commit 3773388 into atom:master Dec 4, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment