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

Fix issue 396 #1156

Closed
wants to merge 4 commits into
base: development
from

Conversation

Projects
None yet
2 participants
@JeffProgrammer
Contributor

JeffProgrammer commented Feb 6, 2015

This fixes issue #396

The regex expression assumed that the programmer would type a character after the beginning of the summary comment line. This now stops it from requiring an extra char.

This will now compile:

///
function pq()
{
}
@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 6, 2015

Contributor

Thanks for looking at this! Regexes... shudders.

Unfortunately I believe the meaning is slightly different now. Previously //// was not parsed ad a doc block, because the grammar specified [^/] immediately after "///". Now the not-slash is optional, and in the case of four slashes, the fourth slash will be parsed as part of the [^\n\r] block.

Reflecting, I think the solution should actually be ("///"([^/][^\n\r]*)?[\n\r]*)+. Will that work?

Contributor

crabmusket commented Feb 6, 2015

Thanks for looking at this! Regexes... shudders.

Unfortunately I believe the meaning is slightly different now. Previously //// was not parsed ad a doc block, because the grammar specified [^/] immediately after "///". Now the not-slash is optional, and in the case of four slashes, the fourth slash will be parsed as part of the [^\n\r] block.

Reflecting, I think the solution should actually be ("///"([^/][^\n\r]*)?[\n\r]*)+. Will that work?

@JeffProgrammer

This comment has been minimized.

Show comment
Hide comment
@JeffProgrammer

JeffProgrammer Feb 11, 2015

Contributor

someone slap me to do this, keep forgetting.

Contributor

JeffProgrammer commented Feb 11, 2015

someone slap me to do this, keep forgetting.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 12, 2015

Contributor

👋

Contributor

crabmusket commented Feb 12, 2015

👋

@JeffProgrammer

This comment has been minimized.

Show comment
Hide comment
@JeffProgrammer

JeffProgrammer Feb 21, 2015

Contributor

should be fixed now. Can someone confirm?

Contributor

JeffProgrammer commented Feb 21, 2015

should be fixed now. Can someone confirm?

@crabmusket crabmusket added the Bug label Feb 21, 2015

@crabmusket crabmusket added this to the 3.7 milestone Feb 21, 2015

@crabmusket crabmusket self-assigned this Feb 21, 2015

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 21, 2015

Contributor

Lol CI is totally broken. Hurrah. Works for me in linux.

Contributor

crabmusket commented Feb 21, 2015

Lol CI is totally broken. Hurrah. Works for me in linux.

@JeffProgrammer

This comment has been minimized.

Show comment
Hide comment
@JeffProgrammer

JeffProgrammer Feb 22, 2015

Contributor

glad to hear its working good on linux too ;)

Contributor

JeffProgrammer commented Feb 22, 2015

glad to hear its working good on linux too ;)

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 22, 2015

Contributor

Shoot. Apparently it doesn't actually fix the issue on Windows. Wat. Investigating.

Contributor

crabmusket commented Feb 22, 2015

Shoot. Apparently it doesn't actually fix the issue on Windows. Wat. Investigating.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 22, 2015

Contributor

This was the best I could do. Tried a bunch of ways to get it to allow plain ///\n but it didn't seem to like any of them. Wonder if there's a deeper issue behind this T_T.

EDIT: Ah. Possibly the deeper issue is 'how the lexer works' :P. Which is kind of a mystery. I tried adding a separate rule to catch //// but it doesn't want to work.

Contributor

crabmusket commented Feb 22, 2015

This was the best I could do. Tried a bunch of ways to get it to allow plain ///\n but it didn't seem to like any of them. Wonder if there's a deeper issue behind this T_T.

EDIT: Ah. Possibly the deeper issue is 'how the lexer works' :P. Which is kind of a mystery. I tried adding a separate rule to catch //// but it doesn't want to work.

@JeffProgrammer

This comment has been minimized.

Show comment
Hide comment
@JeffProgrammer

JeffProgrammer Feb 22, 2015

Contributor

my fix compiles scripts bytecode fine though on windows, What do you mean?

Contributor

JeffProgrammer commented Feb 22, 2015

my fix compiles scripts bytecode fine though on windows, What do you mean?

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 22, 2015

Contributor

Did you test all scripts? With \r\n replaced by \n? For me, your fix didn't work on audio.cs with newlines replaced. See #1220. Because the lexer is greedy, it would always take the optional path you introduced, because that would let it match more input.

Contributor

crabmusket commented Feb 22, 2015

Did you test all scripts? With \r\n replaced by \n? For me, your fix didn't work on audio.cs with newlines replaced. See #1220. Because the lexer is greedy, it would always take the optional path you introduced, because that would let it match more input.

@crabmusket crabmusket closed this Feb 22, 2015

@JeffProgrammer JeffProgrammer deleted the JeffProgrammer:fix-issue-396 branch Mar 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment