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

Jump to bottom

cmd/compile: remove badgerbadgerbadger optimization? #29242

Closed
josharian opened this issue Dec 14, 2018 · 6 comments
Closed

cmd/compile: remove badgerbadgerbadger optimization? #29242

josharian opened this issue Dec 14, 2018 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.13

Comments

@josharian
Copy link
Contributor

From walk.go:

func walkcompareString(n *Node, init *Nodes) *Node {
	// s + "badgerbadgerbadger" == "badgerbadgerbadger"
	if (n.Op == OEQ || n.Op == ONE) && Isconst(n.Right, CTSTR) && n.Left.Op == OADDSTR && n.Left.List.Len() == 2 && Isconst(n.Left.List.Second(), CTSTR) && strlit(n.Right) == strlit(n.Left.List.Second()) {
		r := nod(n.Op, nod(OLEN, n.Left.List.First(), nil), nodintconst(0))
		return finishcompare(n, r, init)
	}

	// cont...

I've always been skeptical that anyone writes code that matches this pattern; I'd like to remove it.

Thoughts/objections?

cc @mdempsky @randall77
@josharian josharian added this to the Go1.13 milestone Dec 14, 2018
@randall77
Copy link
Contributor

Fine by me. Might want to double-check that the stdlib doesn't have any (except a possible test for this optimization).

@mdempsky
Copy link
Member

SGTM

@seebs
Copy link
Contributor

seebs commented Dec 14, 2018

That is a very common pattern... in bourne shell. Where the idiom is test "x$foo" != "x" as a workaround to handle cases where $foo might contain something that would be interpreted as a flag by the test command's option parser. I've also seen it in Makefiles, I think. I agree that it seems vanishingly unlikely to show up in go.

Also why is the function not called walkCompareString with a capital C?

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 14, 2018
@mdempsky
Copy link
Member

Also why is the function not called walkCompareString with a capital C?

It's a subroutine of the vestigially-named walkcompare function. We try to toe a balance between adopting current Go conventions and staying consistent with the current archaic code base.

See also #27167.

@gopherbot
Copy link

Change https://golang.org/cl/163728 mentions this issue: cmd/compile: remove badgerbadgerbadger optimization

@golang golang locked and limited conversation to collaborators Feb 26, 2020
@rsc
Copy link
Contributor

rsc commented Oct 2, 2023

For the record, this was here because of https://groups.google.com/g/golang-nuts/c/7Ks1iq2s7FA/m/rS60bS4aMi4J.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
7 participants
@bradfitz @mdempsky @josharian @rsc @seebs @randall77 @gopherbot