search

atom / language-go

Go language package for Atom
Other
106 stars 65 forks source link

Update keyword.operator.comparison.go regex #145

Closed lzambarda closed 6 years ago

lzambarda commented 6 years ago

Requirements

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

winstliu commented 6 years ago

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 commented 6 years ago

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).

winstliu commented 6 years ago

Seems fine to me. Thanks for the PR!