Skip to content

Support '\r' and '\r\n' line endings, closes scala/bug#5669 - #164

Merged
gourlaysama merged 3 commits into
scala:1.1.xfrom
Philippus:issue-5669
Nov 5, 2018
Merged

Support '\r' and '\r\n' line endings, closes scala/bug#5669#164
gourlaysama merged 3 commits into
scala:1.1.xfrom
Philippus:issue-5669

Conversation

@Philippus

Copy link
Copy Markdown
Member

No description provided.

@Philippus Philippus changed the title Support '\r' and '\r\n' line endings @Philippus Support '\r' and '\r\n' line endings, closes scala/bug#5669 Jul 6, 2018
@Philippus Philippus changed the title @Philippus Support '\r' and '\r\n' line endings, closes scala/bug#5669 Support '\r' and '\r\n' line endings, closes scala/bug#5669 Jul 6, 2018
if (source.charAt(i) == '\n') lineStarts += (i + 1)
lineStarts += source.length
lineStarts += 0 // first line
for (i <- 0 until source.length) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may as well use for i <- 1 until source.length and remove the if i>= 1 checks below

lineStarts += source.length
lineStarts += 0 // first line
for (i <- 0 until source.length) {
if (i >= 1 && source.charAt(i - 1) == '\n') // \n or \r\n

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the majority of the time the last character won't be a \r or \n might be worth binding charAt(i - 1) to a variable to save the extra lookup (dependant on the performance requirements of this code)

@gourlaysama
gourlaysama merged commit c3f8797 into scala:1.1.x Nov 5, 2018
@Philippus
Philippus deleted the issue-5669 branch November 12, 2018 19:15
@SethTisue

Copy link
Copy Markdown
Member

in the Scala community build, this build caused this Twirl test case to start failing: https://github.com/playframework/twirl/blob/68ad7b378be54887a09c5a67792bacc933d17d68/parser/src/test/scala/play/twirl/parser/test/ParserSpec.scala#L187-L189 (referencing https://github.com/playframework/twirl/blob/master/parser/src/test/resources/unclosedBracket2.scala.html)

[play-twirl] [info]   x unclosedBracket2.scala.html
[play-twirl] [error]      '31' is not equal to '32' (ParserSpec.scala:37)

the test file doesn't have any \r or \r\n line endings, so it seems probable it's a real regression? wdyt?

@Philippus

Copy link
Copy Markdown
Member Author

I've reproduced it:
contents of the index generated by genIndex differs:
new: ArrayBuffer(0, 85, 86, 111, 112, 136, 155, 166, 167, 169, 170, 185, 186, 207, 208, 233, 234, 243, 265, 266, 291, 292, 310, 335, 336, 346, 347, 356, 357, 367, 368, 370)
vs
old: ArrayBuffer(0, 85, 86, 111, 112, 136, 155, 166, 167, 169, 170, 185, 186, 207, 208, 233, 234, 243, 265, 266, 291, 292, 310, 335, 336, 346, 347, 356, 357, 367, 368, 370, 370)
will work on a fix.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants