Skip to content

Fix for jehiah/sortdb#4: Use actual copy operator before returning#2

Merged
noam-c merged 2 commits into
masterfrom
copy-bytes-before-unlock
Aug 13, 2015
Merged

Fix for jehiah/sortdb#4: Use actual copy operator before returning#2
noam-c merged 2 commits into
masterfrom
copy-bytes-before-unlock

Conversation

@noam-c

@noam-c noam-c commented Aug 13, 2015

Copy link
Copy Markdown

Also adds a test to ensure that writes after a Search do not modify the
returned buffer (tests that a copy was actually made).

Also adds a test to ensure that writes after a Search do not modify the
returned buffer (tests that a copy was actually made).
Comment thread sorted_db/db.go

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rename s to source and d to dest just for extra clarity, then the comment really isn't even necessary.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not really how Go works:

  1. Comment required for Go Doc
  2. Go style prefers short names: https://github.com/golang/go/wiki/CodeReviewComments#variable-names

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @joshgummersall -- Are you alright with this as is?

@joshgummersall

Copy link
Copy Markdown

One quick comment, then LGTM.

Per the Go guidelines:
- More concise naming for variables that are used immediately
- More descriptive naming for variables that are used further from
their declarations
@TheDahv

TheDahv commented Aug 13, 2015

Copy link
Copy Markdown

lgtm

@joshgummersall

Copy link
Copy Markdown

@noam-c yeah, actually I'd just assign your Go PRs to @TheDahv as he's got them well handled.

@noam-c noam-c assigned TheDahv and unassigned joshgummersall Aug 13, 2015
@noam-c

noam-c commented Aug 13, 2015

Copy link
Copy Markdown
Author

sgtm

noam-c added a commit that referenced this pull request Aug 13, 2015
Fix for jehiah#4: Use actual copy operator before returning
@noam-c
noam-c merged commit 4b1e6b5 into master Aug 13, 2015
@noam-c
noam-c deleted the copy-bytes-before-unlock branch August 13, 2015 21:31
noam-c pushed a commit that referenced this pull request Jan 8, 2016
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