Skip to content

Conversation

@tsenart
Copy link
Contributor

@tsenart tsenart commented Apr 20, 2015

A small code reuse change set following up the work from @ChrisHines in #28.

@ChrisHines
Copy link
Member

LGTM.

Code readability is improved by centralizing the thread safety code and the explanatory comment.

Also, this change subtly improves correctness—even if that was not intended—by copying the backing array of the keyvals passed to log.With. Although I don't expect this to come up very often, the following code works after this change but would have produced unexpected results with the old code.

keyvals := []interface{}{"key", "v1"}
logger = log.With(logger, keyvals...)
keyvals[1] = "v2"
logger.Log(keyvals...)

This introduces an additional allocation when creating the first withLogger, but the result is correct in the general case.

Can we add a test case for the above scenario as part of this PR?

@tsenart
Copy link
Contributor Author

tsenart commented Apr 20, 2015

@ChrisHines: Added the test.

@ChrisHines
Copy link
Member

LGTM (in case it wasn't obvious)

@peterbourgon
Copy link
Member

+12 -13

😎

peterbourgon added a commit that referenced this pull request Apr 22, 2015
log: Reuse code from withLogger in With
@peterbourgon peterbourgon merged commit 457275c into go-kit:master Apr 22, 2015
guycook pushed a commit to codelingo/kit that referenced this pull request Oct 12, 2016
log: Reuse code from withLogger in With
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.

3 participants