search

SmaCCRefactoring / SmaCC

Smalltalk Compiler Compiler : a parser generator
Other
32 stars 15 forks source link

Corrects a misconception about the meaning of stream position #50

Closed apblack closed 6 years ago

apblack commented 6 years ago

A misconception about the meaning of position in a stream created an off-by-one error in the tests for LineNumberStream as well as in the code for LineNumberStream>>columnNumber. This wasn't very noticeable until I started highlighting token ranges in a browser, as well as printing them.

The position of a stream is the index of the most-recently-read character. Initially, a stream is at position 0. After reading one character, it is at position 1, and so on. The column number of a stream must therefor be initially 0, and it becomes 1 only after reading the first character of the stream, and subsequently after reading the first character of a line.

If the most recently-read character is (one of) the newline character(s), then the column position must be 0.

I hope that this commit clarifies the tests, as well as adapting them so that the correct code passes the tests.

ThierryGoubier commented 6 years ago

Hi @apblack , why are you changing SmaCC-Runtime as well?

apblack commented 6 years ago

Because it was wrong! See the comment in the method that returns the columnNumber from a stream position.

" it is not correct to add 1 here; if the newline is at position 7 and the current position is 8, then we are at column 1."

In this case, the old code returned 2.

ThierryGoubier commented 6 years ago

My mistake: my question was about the fact you were also changing the error reporting, and that it doesn't seem to be related.

apblack commented 6 years ago

Thanks for merging. I didn't think that I was changing the error reporting. I am doing so in my private branch, but that should not have been part of the Pull request. It was not in the final version, but perhaps I included it by mistake in an earlier version.