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

x/text: avoid double execution of lookup in lookupAndFormat() #29136

Open
fgm opened this issue Dec 7, 2018 · 2 comments
Open

x/text: avoid double execution of lookup in lookupAndFormat() #29136

fgm opened this issue Dec 7, 2018 · 2 comments
Milestone

Comments

@fgm
Copy link

@fgm fgm commented Dec 7, 2018

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

$ go version
go version go1.11.2 linux/amd64

Does this issue reproduce with the latest release?

  • This is the latest release.
  • code is identical in the master branch

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/fgm/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/fgm/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build766242374=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  • Prepare a simple app with exported strings using //go:generate gotext -srclang=en update -out=catalog/catalog.go -lang=fr
  • Invoke p.Printf("Hello, world") and debug what happens, until stepping into message.lookupAndFormat() until this fragment:
	if p.catContext.Execute(id) == catalog.ErrNotFound {
		if p.catContext.Execute(msg) == catalog.ErrNotFound {
			p.Render(msg)
			return
		}

What did you expect to see?

  • since in most cases id == msg, skip the second execution of p.catContext(msg) if the first execution returned catalog.ErrNotFound

What did you see instead?

  • if p.catContext.Execute(id) returned catalog.ErrNotFound, code performs the same work a second time as p.catContext.Execute(msg)
  • which could be avoided by checking whether id == msg, maybe like:
	if p.catContext.Execute(id) == catalog.ErrNotFound {
		if id == msg || p.catContext.Execute(msg) == catalog.ErrNotFound {
			p.Render(msg)
			return
		}
	}
@gopherbot gopherbot added this to the Unreleased milestone Dec 7, 2018
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 10, 2018

CC: @mpvl

@fgm

This comment has been minimized.

Copy link
Author

@fgm fgm commented Sep 11, 2019

Still present in v0.3.2 (12/2018) and current master HEAD. I could prepare a PR if you so wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.