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

Invoke String when formatting map keys #142

Merged
merged 1 commit into from
May 27, 2019
Merged

Conversation

crawshaw
Copy link
Contributor

This reverts a change introduced in commit 2940eda where the
cmp package stopped calling the String method when printing map
keys. The motivation for the change is unclear, indeed there was
a pre-existing test that String was called on map keys that the
commit changed.

Fixes #141

This reverts a change introduced in commit 2940eda where the
cmp package stopped calling the String method when printing map
keys. The motivation for the change is unclear, indeed there was
a pre-existing test that String was called on map keys that the
commit changed.

Fixes google#141
@dsnet
Copy link
Collaborator

dsnet commented May 27, 2019

I remember now why I avoided String for map keys. It was because of the following situation:

type Int int
func NewInt(i int) *Int { return (*Int)(&i) }
func (i *Int) String() string { return strconv.Itoa(int(*i)) }

m := map[*MyInt]string{
    NewInt(5): "foo",
    NewInt(5): "bar",
}

The stringer for both keys prints to the same thing, which is slightly different from what the map is comparing on (the pointers).

However, that situation should be fixed by a more general change in the future that avoids stringer altogether if the outputs are ambiguous:

// TODO: Enforce unique outputs?
// * Avoid Stringer methods if it results in same output?
// * Print pointer address if outputs still equal?

@dsnet dsnet changed the title cmp: invoke stringer on map keys Invoke String on map keys May 27, 2019
@dsnet dsnet changed the title Invoke String on map keys Invoke String when formatting map keys May 27, 2019
@dsnet dsnet merged commit 917e382 into google:master May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stringer not used for map keys
2 participants