client: pass `serverResponse` around by value

[tamird]

Removes a bunch of impossible and useless nil checks.

Signed-off-by: Tamir Duberstein tamird@gmail.com

[tamird]

@thaJeztah @calavera can you guys take a look? You appear to be the main contributors to client/request.go.

[cpuguy83]

I like it, LGTM.

[LK4D4]

@tamird It's sorta against the "rules"(like you're supposed to use pointer if your struct contains pointer). I think that it's impossible to have nil serverResp because it's instantiated at the top of sendClientRequest, so you can just remove nil-checks.

[tamird]

Why are you supposed to use a pointer if the struct contains a pointer?

On Aug 11, 2016 15:00, "Alexander Morozov" notifications@github.com wrote:

@tamird https://github.com/tamird It's sorta against the "rules"(like you're supposed to use pointer if your struct contains pointer). I think that it's impossible to have nil serverResp because it's instantiated at the top of sendClientRequest, so you can just remove nil-checks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/docker/engine-api/pull/356#issuecomment-239258137, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdsPOTMMFUJ4_cDA12c44qefFXXnYAUks5qe3FpgaJpZM4JgMIr .

[LK4D4]

@tamird It's code style and it's easier for GC to track such pointers I think.

[tamird]

That is incorrect. Passing this around by value is definitely better from the GC perspective, since the structs will be copied on the stack and not touch the heap at all.

[LK4D4]

@tamird I thought that escape analysis will put them on heap anyway if they contain pointers, maybe I'm wrong. I still think that it's better to use smaller diff with same effect, because serverResp never can be nil.

[tamird]

@LK4D4 escape analysis will put the thing pointed to on the heap, not the thing containing the pointer.

No, it is better to have something which is more type safe (which this is, because it doesn't use a nullable pointer at all).

[LK4D4]

@tamird ok, you're right LGTM