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

Shouldn't payload be a pointer #75

Closed
nexthack opened this issue Aug 20, 2017 · 4 comments
Closed

Shouldn't payload be a pointer #75

nexthack opened this issue Aug 20, 2017 · 4 comments

Comments

@nexthack
Copy link
Contributor

At this line,shouldn't the payload be a pointer.

Similarly at this line ,I think the data should be a pointer as we are passing p.payload.

@ruflin
Copy link
Member

ruflin commented Aug 21, 2017

Hi @crackerplace. Happy to see you get involved with apm-server.

The first payload you mentioned is an interface. So I think passing it here as is should work as expected or do you see any unwanted side affects?

For the second line you mentioned it's passed as value as we only extract the data and don't modify it. Passing it by pointer would also work but I prefer in general if not pointer is need to pass a copy except it would become a performance issue.

@nexthack
Copy link
Contributor Author

nexthack commented Aug 21, 2017

@ruflin thanks,Would be happy to contribute :-)

Sounds fine as we are passing a pointer to the struct payload which satisfies pr.Payload.

A simple better change would be to make payload as lower case at line as we are anywayz assigning it to an interface and need not be exported.It helps in understanding as well.What do you think ?

@ruflin
Copy link
Member

ruflin commented Aug 22, 2017

Making it private SGTM assuming we don't need it anywhere else. Can you open a PR?

@nexthack
Copy link
Contributor Author

@ruflin Sure.Will open.

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

No branches or pull requests

2 participants