code review 1636043: go/scanner: report illegal escape sequences

go/scanner: report illegal escape sequences

[gri, 2010-06-09 21:59:52]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[r, 2010-06-09 22:11:48]

LGTM

http://codereview.appspot.com/1636043/diff/2001/3002
File src/pkg/go/scanner/scanner.go (right):

http://codereview.appspot.com/1636043/diff/2001/3002#newcode384
src/pkg/go/scanner/scanner.go:384: S.error(pos, "escape sequence is invalid
Unicode code point")
i would echo the value that's out of range. that helps when there are multiple
values  on a line

[gri, 2010-06-09 22:17:27]

On Wed, Jun 9, 2010 at 3:11 PM, <r@golang.org> wrote:

> LGTM
>
>
> http://codereview.appspot.com/1636043/diff/2001/3002
> File src/pkg/go/scanner/scanner.go (right):
>
> http://codereview.appspot.com/1636043/diff/2001/3002#newcode384
> src/pkg/go/scanner/scanner.go:384: S.error(pos, "escape sequence is
> invalid Unicode code point")
> i would echo the value that's out of range. that helps when there are
> multiple values  on a line


gofmt provides precise column information, so the offending value is clearly
identified
(I was thinking of printing it as well, but at the moment the error function
only accepts a string and I didn't want to use Sprintf. At some point I will
make the error reporting function more flexible).

>
>
> http://codereview.appspot.com/1636043/show
>

[gri, 2010-06-09 22:19:28]

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

go/scanner: report illegal escape sequences

R=golang-dev, r
CC=golang-dev
http://codereview.appspot.com/1636043

[r2, 2010-06-09 22:22:57]

On Jun 9, 2010, at 3:17 PM, Robert Griesemer wrote:

> On Wed, Jun 9, 2010 at 3:11 PM, <r@golang.org> wrote:
> LGTM
> 
> 
> http://codereview.appspot.com/1636043/diff/2001/3002
> File src/pkg/go/scanner/scanner.go (right):
> 
> http://codereview.appspot.com/1636043/diff/2001/3002#newcode384
> src/pkg/go/scanner/scanner.go:384: S.error(pos, "escape sequence is
> invalid Unicode code point")
> i would echo the value that's out of range. that helps when there are
> multiple values  on a line
> 
> gofmt provides precise column information, so the offending value is clearly
identified
> (I was thinking of printing it as well, but at the moment the error function
only accepts a string and I didn't want to use Sprintf. At some point I will
make the error reporting function more flexible).

i'm not a fan of the 'column' idea in any form, but it's clear i'm in the
minority.

-rob

[rsc, 2010-06-09 22:57:50]

> gofmt provides precise column information, so the offending value is clearly
> identified

it is identified, but not clearly.
having to count the columns is much
more work and less clear for the
programmer than being told what is wrong.
if you don't want to use Sprintf there is
always strconv, which is already imported.

[gri, 2010-06-09 23:07:46]

On Wed, Jun 9, 2010 at 3:57 PM, Russ Cox <rsc@golang.org> wrote:

> > gofmt provides precise column information, so the offending value is
> clearly
> > identified
>
> it is identified, but not clearly.
> having to count the columns is much
> more work and less clear for the
> programmer than being told what is wrong.
>

I think we are nitpicking here. If there are indeed more than one such
escape value in the string it's about as hard to look at the column counter
of the editor (most have one) as it is to manually compare 2 hex numbers.


> if you don't want to use Sprintf there is
> always strconv, which is already imported.
>

As I said before, at some point the error function should report more
information but for now this is good enough
- Robert