Skip to content
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.

Sign up for GitHub

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

Jump to bottom

go/scanner: potentially redundant code in Scanner.next() #63996

Closed
nodir-t opened this issue Nov 8, 2023 · 1 comment
Closed

go/scanner: potentially redundant code in Scanner.next() #63996

nodir-t opened this issue Nov 8, 2023 · 1 comment

Comments

@nodir-t
Copy link

nodir-t commented Nov 8, 2023 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.20.6 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/ntx/Library/Caches/go-build"
GOENV="/Users/ntx/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/ntx/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/ntx/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.6"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/d2/c04j5b9129vbwrk4wbwy7mjh0000gr/T/go-build2833291424=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I was reading Go source code. The AddLine call here seems to be a noop:

go/src/go/scanner/scanner.go

Lines 82 to 86 in c9888bd

s.offset = len(s.src)
if s.ch == '\n' {
s.lineOffset = s.offset
s.file.AddLine(s.offset)
}

This is because we effectively pass file size as offset and AddLine implementation ignores the call if offset >= size:

go/src/go/token/position.go

Lines 135 to 137 in c9888bd

if i := len(f.lines); (i == 0 || f.lines[i-1] < offset) && offset < f.size {
f.lines = append(f.lines, offset)
}

Note that we have a guarantee that File.size == len(src):

go/src/go/scanner/scanner.go

Lines 125 to 127 in c9888bd

if file.Size() != len(src) {
panic(fmt.Sprintf("file size (%d) does not match src len (%d)", file.Size(), len(src)))
}

I removed that line locally and the src/all.bash tests still pass. Happy to send a PR to remove that line, if you are OK with this.

cc @griesemer
@nodir-t nodir-t changed the title go/scanner: potentially dead code in Scanner.next() go/scanner: potentially redundant code in Scanner.next() Nov 8, 2023
@griesemer
Copy link
Contributor

Thanks for finding this, but let's leave this code alone, for the following reasons:

  1. As is, the lines 83-86 match the lines 62-65 (as they should); and line 81 could probably be the same as line 61. The code could probably be restructered so that this code section appears only once (by modifying the if on line 60). If the AddLine on line 85 is removed, this will be harder to see. In other words, the symmetry here helps understanding.

  2. The AddLine on line 85 is called once per file at most (unless one calls next() repeatedly at EOF). This has virtually zero impact on anything.

  3. I believe your analysis is correct, but there's always a chance that one overlooks something. This code has been running fine for a decade - let's not fiddle with it, especially since there's no bug here nor is there a performance benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@griesemer @nodir-t