code review 4094041: scanner: error handler must be provided to Init

scanner: error handler must be provided to Init

Init may report an error on the first character and
thus one needs an ability to set the error handler
for Init. Was a design bug.

Added corresponding test cases and better documentation.
Also: Fixed a subtle infinite loop exposed by one of the
new test cases.

Fixes issue 1380.

[gri, 2011-01-21 02:06:32]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2011-01-21 22:18:15]

LGTM

Okay if you want, but I think the API would be nicer
to use if you could find a way to keep it as it is.

http://codereview.appspot.com/4094041/diff/5001/src/pkg/scanner/scanner.go
File src/pkg/scanner/scanner.go (right):

http://codereview.appspot.com/4094041/diff/5001/src/pkg/scanner/scanner.go#ne...
src/pkg/scanner/scanner.go:179: // itself. ErrorCount is set to 0, Mode is set
to GoTokens, and Whitespace is
s/itself/s/

(itself would be Init)

http://codereview.appspot.com/4094041/diff/5001/src/pkg/scanner/scanner.go#ne...
src/pkg/scanner/scanner.go:180: // set to GoWhitespace. Init attemps to read
ahead and may report a read error
This is okay if you insist, but it seems awkward to
change a public API because of an internal implementation
detail (s.ch).

An alternative fix would be to allow s.ch to be -1 to
mean "no lookahead yet", implement that in Peek, and
make Next and Scan call Peek instead of grabbing s.ch
directly.

This would have the benefit of being able to scan an
obviously self-delimiting thing like a quoted string
without any final lookahead.

[gri1, 2011-01-24 22:08:23]

I've fixed this as suggested with with no API change (well, the scanner position
is undefined after Init, but the scanner position implementation because of
issue 1327). Next CL.

http://codereview.appspot.com/4094041/diff/5001/src/pkg/scanner/scanner.go
File src/pkg/scanner/scanner.go (right):

http://codereview.appspot.com/4094041/diff/5001/src/pkg/scanner/scanner.go#ne...
src/pkg/scanner/scanner.go:179: // itself. ErrorCount is set to 0, Mode is set
to GoTokens, and Whitespace is
On 2011/01/21 22:18:15, rsc wrote:
> s/itself/s/
> 
> (itself would be Init)

Done.

http://codereview.appspot.com/4094041/diff/5001/src/pkg/scanner/scanner.go#ne...
src/pkg/scanner/scanner.go:180: // set to GoWhitespace. Init attemps to read
ahead and may report a read error
On 2011/01/21 22:18:15, rsc wrote:
> This is okay if you insist, but it seems awkward to
> change a public API because of an internal implementation
> detail (s.ch).
> 
> An alternative fix would be to allow s.ch to be -1 to
> mean "no lookahead yet", implement that in Peek, and
> make Next and Scan call Peek instead of grabbing s.ch
> directly.

Changed.
> 
> This would have the benefit of being able to scan an
> obviously self-delimiting thing like a quoted string
> without any final lookahead.

I think that needs a bit more of a change because then sometimes the position is
one ahead and sometimes it is not.
Will fix that as part of issue 1327 (which may require an API change after all).

[rsc, 2011-01-24 22:10:32]

LGTM

[gri, 2011-01-24 22:12:30]

*** Submitted as http://code.google.com/p/go/source/detail?r=65b92948f9ee ***

scanner: error handler must be provided to Init

Init may report an error on the first character and
thus one needs an ability to set the error handler
for Init. Was a design bug.

Added corresponding test cases and better documentation.
Also: Fixed a subtle infinite loop exposed by one of the
new test cases.

Fixes issue 1380.

R=rsc, gri
CC=golang-dev
http://codereview.appspot.com/4094041