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

fmt: pp should implement WriteString() #20786

Closed
pierrre opened this issue Jun 24, 2017 · 16 comments
Closed

fmt: pp should implement WriteString() #20786

pierrre opened this issue Jun 24, 2017 · 16 comments

Comments

@pierrre
Copy link

@pierrre pierrre commented Jun 24, 2017

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

1.9beta1

What did you do?

In my code, I'm implementing fmt.Formatter.
In order to write a string to the fmt.State, I'm calling io.WriteString().
I know that this function has an optimization that allows to call WriteString() if w implements it.
Sadly, in my case the "real" type of fmt.State is fmt.pp.
fmt.pp only has Write().

What did you expect to see?

I think we should add:

func (p *pp) WriteString(s string) (ret int, err error) {
	p.buf.WriteString(s)
	return len(s), nil
}

in my case, it greatly improve performance and reduces memory allocation.
We could also implement WriteByte() and WriteRune(), since fmt.buffer has these methods.

@bradfitz bradfitz added this to the Go1.10 milestone Jun 24, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 24, 2017

Sounds reasonable.

@nikhita
Copy link

@nikhita nikhita commented Jun 25, 2017

I want to contribute to Go and noticed the Help Wanted label. If no one is working on it, I'd like to take it! :)

@martisch
Copy link
Contributor

@martisch martisch commented Jun 25, 2017

io.WriteString() uses a runtime check so it looks like it should work without changing the State Interface definition. For go2 we could then think about wether it makes sense to update the State Interface.

@nikhita go ahead if you want too. Please add r@golang.org and moehrmann@google.com as reviewers. We can review before go1.10 but we can only submit when the code tree opens for go1.10 start of August.

@nikhita
Copy link

@nikhita nikhita commented Jun 25, 2017

@martisch will do that, thank you.

@SCKelemen
Copy link
Contributor

@SCKelemen SCKelemen commented Jul 7, 2017

@martisch
Copy link
Contributor

@martisch martisch commented Jul 10, 2017

@nikhita
did you have time to start your first CL for go?
Any blockers encountered that we can help with?

@nikhita
Copy link

@nikhita nikhita commented Jul 10, 2017

@martisch
sorry, was busy at work - didn't have time to work on it.
But will try working on it on my flight to GopherCon now. :)
Maybe I can even ask any questions I have during the Go contributor workshop at GopherCon? :)

@martisch
Copy link
Contributor

@martisch martisch commented Jul 10, 2017

no problem. Asking at the contributor workshop sounds good.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 15, 2017

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

@rajathagasthya
Copy link
Contributor

@rajathagasthya rajathagasthya commented Sep 1, 2017

Just checking if this is still active. I see in the CL that it needs tests. I can help with that, if need be.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 4, 2017

Change https://golang.org/cl/61430 mentions this issue: fmt: Implement pp.WriteString() method

@stemar94
Copy link

@stemar94 stemar94 commented Sep 4, 2017

I mailed another way on how to resolve this. gopherbot will notice soon.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 4, 2017

Change https://golang.org/cl/61450 mentions this issue: fmt: make buffer satisfy io interfaces

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 4, 2017

Change https://golang.org/cl/61451 mentions this issue: fmt: make pp a composition with buffer

@robpike
Copy link
Contributor

@robpike robpike commented Sep 7, 2017

Adding WriteString makes some sense, but please let's not open up an internal type to the full pressure of external I/O interfaces.

@martisch
Copy link
Contributor

@martisch martisch commented Sep 7, 2017

Following up here instead of on the CL discussion.

I would also favor to leave the internal buffer methods unexposed and only implement WriteString on pp as the proposal suggested. As far as i know there has been not yet been data or a request to add more io methods beyond WriteString. Keeping the api footprint low imposes less constraints on internals for future changes and avoids needing to implement them explicitly on pp later if buffer is replaced by something with different interfaces for internal optimization.

If it is decided to expose all the io methods from the buffer it would be good to make sure that there are no regressions due to the compiler not optimizing all the overhead away for the internal use of those methods (e.g. due to inline budget changes).

@gopherbot gopherbot closed this in 8802b18 Sep 20, 2017
@golang golang locked and limited conversation to collaborators Sep 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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