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

benchstat: accept io.Writer in FormatHTML, not *bytes.Buffer #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nvanbenschoten
Copy link

This brings the API in-line with that of FormatText and FormatCSV and
allows it to be used without necessitating an intermediate buffer. The
change is fully backwards compatible and doesn't impose any potential
performance cost because html.Template.Execute accepts an io.Writer,
so any bytes.Buffer passed by reference to FormatHTML was already
being forced to escape onto the heap.

This might have been done this way to avoid questions about how the
function should handle errors from Writer, but we already ignore those
questions and panic in FormatCSV, so it seems reasonable that we would
do the same thing here.

This brings the API in-line with that of FormatText and FormatCSV and
allows it to be used without necessitating an intermediate buffer. The
change is fully backwards compatible and doesn't impose any potential
performance cost because `html.Template.Execute` accepts an io.Writer,
so any `bytes.Buffer` passed by reference to FormatHTML was already
being forced to escape onto the heap.

This might have been done this way to avoid questions about how the
function should handle errors from Writer, but we already ignore those
questions and panic in `FormatCSV`, so it seems reasonable that we would
do the same thing here.
@gopherbot
Copy link
Contributor

This PR (HEAD: c5a59e8) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/perf/+/211320 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

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

Successfully merging this pull request may close these issues.

3 participants