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

C++ SDK: Use const-reference in WatchGameServer #941

Closed
roberthbailey opened this issue Jul 23, 2019 · 6 comments · Fixed by #951

Comments

@roberthbailey
Copy link
Member

commented Jul 23, 2019

From #934:

Is it intentional to pass a GameServer object by value? I don't know if this is an expensive type to copy, but it looks like accepting this object by const-reference may be worthwhile.

We should consider changing the function signature of

grpc::Status SDK::WatchGameServer(
    const std::function<void(agones::dev::sdk::GameServer)>& callback) { ... }

Since this would be a breaking change to the CPP SDK, I'm marking this for milestone 0.12.0 and we should resolve to do this (or leave it) ASAP.

@devjgm

@roberthbailey

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

@Kuqd / @markmandel - thoughts?

@markmandel

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

I've no strong opinions. I basically wrote the C++ SDK as a sacrificial draft. Very happy to defer to opinions that have more C++ experience than me.

@devjgm

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

A couple questions:

  1. Does the GameServer argument to the callback need to be mutated? Or could it be const?
  2. Is GameServer a small and cheap type that is most efficiently passed by value?

Depending on the answers to the above, I'd suggest a signature like:

grpc::Status SDK::WatchGameServer(
    const std::function<void(const agones::dev::sdk::GameServer&)>& callback);
@roberthbailey

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

  1. My understanding is that it should not need to be mutated as changes to it would not be reflected back into k8s (so mutating it would only be confusing).

  2. I'm not sure where the cut-off is for small/cheap, but GameServer is a generated proto type, so I'm guessing the answer would be no.

@markmandel

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

I can't think of any reason you would want to mutate the gameserver

@roberthbailey

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

If there aren't any dissenting opinions by the end of the day tomorrow I'll send a PR to change the function signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.