-
Notifications
You must be signed in to change notification settings - Fork 191
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
Multi-line comment support #128
base: master
Are you sure you want to change the base?
Conversation
Also added multiline support to the parser. |
I get this error when trying to run moonc after your patch. Here's a similar issue with that popped up with lpeg 0.12: mascarenhas/cosmo#5 bin/moonc moon/ moonscript/
lua: ./moonscript/parse.lua:395: bad argument #2 to '__div' (invalid replacement value)
stack traceback:
[C]: in function '__div'
./moonscript/parse.lua:395: in function 'build_grammar'
./moonscript/parse.lua:658: in function 'string'
bin/moonc:121: in function 'compile_file'
bin/moonc:169: in function 'compile_and_write'
bin/moonc:402: in main chunk
[C]: in ?
Makefile:12: recipe for target 'compile' failed
make: *** [compile] Error 1 |
The error message refers to this line: Comment = P"--" * (V"NoSpaceLuaString" / 0 * Space^-1 + (1 - S"\r\n")^0 * #Stop), And mentions a "bad argument 2 to _div" which I guess refers to the 0 which I placed there so that all captures made by the LuaString rule would be dropped (otherwise the compiler thinks it found a "string" line decorator if the comment was at the end a line). This behavior is documented on the official lpeg page and works fine for me using a fresh lpeg v0.12 from LuaRocks. |
What's the status on merging this @leafo? |
The line number fix was manually merged in. The multiline comment was not. I've been lingering on adding it because it makes parsing about 20% slower if I remember correctly. I wanted to take some time to see if is a more efficient implementation. |
Any update on this? |
It would be pretty nice to have multi-line comments: what sort of % performance hit would be acceptable? |
Issue #127 should disappear with this. If
DelayedLine
ever becomes implemented some changes will be necessary, but until then everything should work nicely.