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.

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

bufio: potentially unnecessary "continue" in bufio/scan.go #15712

Closed
roylee0704 opened this issue May 17, 2016 · 2 comments
Closed

bufio: potentially unnecessary "continue" in bufio/scan.go #15712

roylee0704 opened this issue May 17, 2016 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@roylee0704
Copy link

roylee0704 commented May 17, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go1.6.2 darwin/amd64
  2. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/roylee/dev/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.6.2/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.6.2/libexec/pkg/tool/darwin_amd64"
GO15VENDOREXPERIMENT="1"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
CXX="clang++"
CGO_ENABLED="1"
  1. What did you do?

bug: bufio/scan/scan.go, line 202.

It is not necessarily to continue, as in the next iteration, none of the codes earlier needs to be touched. It creates unnecessarily overheads to run splitFunc without filling up data from reader, first.

I have ran scan_test.go without continue, to my surprise, they are all passed! At the very least, if it belongs there, they should add a test case that shows it. I'm generally of the opinion that if small-but-severe changes to the code don't cause tests to fail, the tests are incomplete.

In my opinion, removing a continue definitely qualifies.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
4. What did you expect to see?
5. What did you see instead?

@roylee0704 roylee0704 changed the title Potential Bug: Unnecessarily overheads at scan.go Potential Bug: Unnecessarily overheads at bufio/scan.go May 17, 2016
@roylee0704 roylee0704 changed the title Potential Bug: Unnecessarily overheads at bufio/scan.go Unnecessarily overheads at bufio/scan.go May 17, 2016
@roylee0704 roylee0704 changed the title Unnecessarily overheads at bufio/scan.go Potential Bug: Unnecessarily overheads at bufio/scan.go May 17, 2016
@bradfitz bradfitz changed the title Potential Bug: Unnecessarily overheads at bufio/scan.go bufio: potentially unnecessary "continue" in bufio/scan.go May 17, 2016
@bradfitz bradfitz added this to the Go1.8Maybe milestone May 17, 2016
@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 10, 2016

I agree the code remains correct if the continue is removed and that it eliminates a redundant call to s.split. It seems fine to remove. @quentinmit, want to do this?

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 10, 2016
@gopherbot
Copy link

CL https://golang.org/cl/31715 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants