Skip to content

Lua scanner -- Meta-commit. - #22

Merged
korny merged 1 commit into
rubychan:masterfrom
Quintus:luascanner-onecommit
Jun 22, 2013
Merged

Lua scanner -- Meta-commit.#22
korny merged 1 commit into
rubychan:masterfrom
Quintus:luascanner-onecommit

Conversation

@Quintus

Copy link
Copy Markdown

OK, here’s the "meta-commit" for the Lua scanner you requested.

But I don’t think it’s unusual to have a pull request containing multiple commits. In fact it’s what I see most times when I look at pull requests (GitHub even encourages to add even more commits to a pull requests after it has been opened).

Vale,
Quintus

This commit is a super-commit containing all the subcommits for
implementing the Lua scanner.
@Quintus

Copy link
Copy Markdown
Author

Any opinion? Plans for merging, rejecting?

Valete,
Quintus

@Quintus

Copy link
Copy Markdown
Author

If noone comments, I cannot improve the code. And as far as I know there’s always something to improve ;-)

Valete,
Quintus

@trans

Copy link
Copy Markdown

@Quintus I assume you've used your patch and know that it does the job. So that being so, @rubychan please merge!

@korny

Copy link
Copy Markdown
Member

Quintus: Sorry for taking so long to answer. I will look at it, Lua is on top of my wish list :)

@korny

Copy link
Copy Markdown
Member

Wow, your scanner seems almost perfect! It had no problems with the example file I threw at it (http://svn.rubychan.de/coderay-scanner-tests/trunk/lua/example.in.lua).

I fixed a Ruby 1.8 incompatibility (leading method dots…I was against that change in ruby-core ;-), and escaped the /{/ and /}/ regexps (confuses my editor).

Please run rake test:scanner:lua on the lua-scanner branch and take a look at the output (test/scanners/lua/example.actual.html). I have some minor questions about details like local variables and tables.

I wonder, why did you choose to use handle_state_* methods instead of the case @state / when :state approach all other scanners use? I tested the old variant (see the lua-scanner-faster branch) and it's about 20% faster on MRI 1.9.3. Is there a strong benefit I missed?

@ghostghost assigned kornyJun 25, 2012
@Quintus

Copy link
Copy Markdown
Author

Quintus: Sorry for taking so long to answer.

You see I’m no better ;-). We all are forced to spent our time elsewhere...

Wow, your scanner seems almost perfect!

Thanks ;-). Given my initial motivation of writing this scanner (I justed wanted to highlight Lua code in a scripting manual generated via kramdown and was surprised that CodeRay had no Lua support) this surprises me as well. Of course I’ve tested some obscure code with the scanner (did you know all the ways you can write numbers in Lua?), but never with such a big file. Thanks for testing!

I fixed a Ruby 1.8 incompatibility (leading method dots…I was against that change in ruby-core ;-)

OK... I’ve stopped caring about 1.8 compatibility as this version will soon officially be discouraged.

escaped the /{/ and /}/ regexps (confuses my editor).

So your editor is not CodeRay-powered! What a shame! :-D

Please run rake test:scanner:lua on the lua-scanner branch

$ rake test:scanner:lua
svn up test/scanners
Updating 'test/scanners':
Revision 68.
/opt/rubies/ruby-1.9.3-p194/bin/ruby ./test/scanners/suite.rb lua
!! Suite /home/quintus/Programmieren/Projekte/coderay/test/scanners/lua/suite.rb not found

I’ve then created the file with this content:

class Lua < CodeRay::TestCase
end

take a look at the output (test/scanners/lua/example.actual.html)

(540) [11:43:25 quintus@hades] ~/Programmieren/Projekte/coderay
$ diff -u test/scanners/lua/example.expected.html test/scanners/lua/example.actual.html (541) [11:44:24 quintus@hades] ~/Programmieren/Projekte/coderay
$

Not sure what you mean, looks quite identical to me. Could you be more specific?

I wonder, why did you choose to use handle_state_* methods instead of the case @State / when :state approach all other scanners use? I tested the old variant (see the lua-scanner-faster branch) and it's about 20% faster on MRI 1.9.3. Is there a strong benefit I missed?

Mh, I confess to not have thought that CodeRay not only aims to be the best, but also the fastest syntax highlighter out there in Ruby land... I simlpy dislike super-long case statements as they decrease readability which I favour over performance. Plus, it is easier to make recursive calls with methods rather than case statements. I can try to rewrite the scanner using case instead of send if you want.

Vale,
Quintus

@korny

korny commented Jul 1, 2012

Copy link
Copy Markdown
Member

I’ve stopped caring about 1.8 compatibility as this version will soon officially be discouraged.

I am prepared to drop 1.8 compat after CodeRay 1.1, but for the time being, I would like to keep it.

So your editor is not CodeRay-powered! What a shame! :-D

Technically, they have to be escaped. It seems TextMate is trying to be too clever when it parses RegExps.

!! Suite /home/quintus/Programmieren/Projekte/coderay/test/scanners/lua/suite.rb not found

Fixed. Forgot how to use SVN.

Not sure what you mean, looks quite identical to me. Could you be more specific?

It's identical because it creates *.actual.html on the first run. "expected" is what I got – I normally take the time to read/scan through it with my own eyes in the browser to spot errors. Then I fix them, run the suite again, and in the end, I can submit the good result as "expected".

So, in your case, look at either of the files and see if you can spot any problems.

I simlpy dislike super-long case statements as they decrease readability which I favour over performance.

I'm sure we could talk over this forever, exchanging valid points. Can you accept that I want to keep the codebase single-style? As in, all scanners should look/work the same, anticipating another batch rewrite in the future.

@korny

korny commented Jul 1, 2012

Copy link
Copy Markdown
Member

Plus, it is easier to make recursive calls with methods rather than case statements.

Not if you store the state in a variable anyway.

I can try to rewrite the scanner using case instead of send if you want.

I already did that in lua-scanner-faster :-)

@nathany

Copy link
Copy Markdown
Contributor

One thing I'm wondering about is the addition of the "table" Style. Afaik, the tables in Lua are somewhat similar to the Arrays in PHP, supporting both arrays and hashes/associative arrays. Since styles are across languages in Coderay, I was wondering if there is a more appropriate name for the style itself? (or another style that could be reused)

@korny

korny commented Jul 3, 2012

Copy link
Copy Markdown
Member

I like the shades-of-blue trick. We could use that for JSON, too. Adding a new token kind is fine; maybe we find a more generic name?

@nathany

Copy link
Copy Markdown
Contributor

Yah, the shades-of-blue trick is cool. It reminds me of how Xcode highlights indentation with shades of gray. Maybe it could be named a :dictionary or :dict though (which I believe is more general than hash map, and shorter than associative array).

It's been a while since I've looked at Lua, but I'm not seeing any obvious issues with the tokenizer in lua/example.expected.html. A few concerns that look related to the colors.

  • There appears to be a Unicode issue with Mark/Ovídio/Scuri on line ~1243. Funny characters.
  • Line 3648 looks a little funny, but I think it's just the colors selected and not a token issue. io.write('#include "il.h"\n\n\n')
  • Is it intentional that strings "..." '...' [[...]] are purple on grey when inside tables rather than the same as other strings, or is this just a side effect of the hsla stuff?

I've merged master and faster into lua-scanner, along with escaping some additional regex expressions. I'm working to convert a Lua blog from WordPress to NestaCMS, so I'll be referencing this branch.

Nathan.

@nathany

Copy link
Copy Markdown
Contributor

Changed :table to :map, which is better for reuse in other languages, and won't be confused with :table based html rendering. 279348d

example.expected.raydebug needs to be updated

@nathany

Copy link
Copy Markdown
Contributor

This article has some slightly bizarre code that is reporting as errors with the Lua scanner: http://www.luanova.org/porting-lua-to-risc-os.

@korny

Copy link
Copy Markdown
Member

Good finding! Can you add that code to the Lua test suite?

@ghostghost assigned nathanyNov 11, 2012
@nathany

Copy link
Copy Markdown
Contributor

I'm not actually sure if that is valid Lua code, I just assume it is because someone published it on my web site :-)

@korny

Copy link
Copy Markdown
Member
>luac--- Fred, tiny wimp program using error dialoguerequire"wimp.task"dolocaldim,! inriscosluac: stdin:6: '<name>' expectednear'!'

@ghostghost assigned kornyDec 22, 2012
@Phrogz

Copy link
Copy Markdown

FWIW I tried manually patching the current gem with the code from this commit, and the process using highlighting hung when I tried to highlight a Lua snippet. In case it was snippet-specific, here's what I was coloring:

require 'os'
local c = os.clock
function r1(i,n) return math.floor(i/n+0.5)*n end
function r2(i,n) local m=n/2; return i+m - (i+m)%n end
local t=c(); for i=1,12345678 do r1(i,20) end; print(c()-t) --> 2.007
local t=c(); for i=1,12345678 do r2(i,20) end; print(c()-t) --> 1.45

@korny

korny commented Mar 5, 2013

Copy link
Copy Markdown
Member

Thanks! Can you post the error message?

@Phrogz

Copy link
Copy Markdown

@korny I wish I could provide an error message, but there was none. Literally the process took 100% CPU and stayed there until I killed it. :/

@Phrogz

Copy link
Copy Markdown

FWIW, I've written my own simple Lua scanner here: https://github.com/Phrogz/coderay/blob/master/lib/coderay/scanners/lua.rb

(I started out writing a recursive descent parser for Lua before realizing that the EBNF grammar supplied is left-recursive in many locations. So I went with the one huge regex approach that covers 90% of the formatting I wanted.)

@korny

Copy link
Copy Markdown
Member

example.expected.raydebug needs to be updated

Done!

Wow, activity after a few months ;) Can we still get this ready for CodeRay 1.1? I'm trying to…

@kornykorny mentioned this pull request Jun 22, 2013
@korny

Copy link
Copy Markdown
Member

@Phrogz: I added your snipped to the test suite, it runs smoothly. Can't produce a bug.

@korny

Copy link
Copy Markdown
Member

@nathany:

There appears to be a Unicode issue with Mark/Ovídio/Scuri on line ~1243. Funny characters.

Yes, it was Latin-1 or something. Fixed it.

Line 3648 looks a little funny, but I think it's just the colors selected and not a token issue.

Not sure what you mean:
screenshot_01

Is it intentional that strings "..." '...' [[...]] are purple on grey when inside tables rather than the same as other strings, or is this just a side effect of the hsla stuff?

Funny, this is fixed by moving the .map styles before the .string styles (where they actually belong, alphabetically, now that they are named "map" instead of "table"). The CSS is really brittle; we have to fix this in the future. Possibly using .map > .content or something.

I prefer the red strings:
screenshot_01

This article has some slightly bizarre code that is reporting as errors with the Lua scanner: http://www.luanova.org/porting-lua-to-risc-os. I'm not actually sure if that is valid Lua code, I just assume it is because someone published it on my web site :-)

Lua says no:

lua: lua/fred.in.lua:4: '<name>' expected near '!'

Still, I added it as an example.

@korny

Copy link
Copy Markdown
Member

Yay, I have no issue left. The scanner is a bit slower than the Ruby scanner, but it's complex and handles complicated Lua code very well. We can optimize it later.

Thank you, @Quintus, for your awesome work on this. It's sad that my laziness prevented it from being merged earlier. CodeRay 1.1 will have great Lua support thanks to you!

@kornykorny mentioned this pull request Jun 22, 2013
@korny
korny merged commit 501df76 into rubychan:masterJun 22, 2013
@nathany

Copy link
Copy Markdown
Contributor

Thanks @korny

@kornykorny mentioned this pull request Jun 22, 2013
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Quintus@trans@korny@nathany@Phrogz