search

nats-io / nats-server

High-Performance server for NATS.io, the cloud and edge native messaging system.
https://nats.io
Apache License 2.0
16.03k stars 1.41k forks source link

[CHANGE] change value receiver to pointer receiver #5965

Closed Coen90 closed 1 month ago

Coen90 commented 1 month ago

Related to #5964

Signed-off-by: Hyuntae Park bht9011@gmail.com

kozlovic commented 1 month ago

@Coen90 readCacheFlag is a type alias for a uint16. There is no reason to make this a pointer. The receiver would be equivalent in C code to something like: isSet(flag uint16), which is perfectly fine. I think the PR and corresponding issue could be closed if you find the explanation satisfactory.

Coen90 commented 1 month ago

This code works perfectly, but I suggested it because A Tour of Go advises against mixing them. If you think there is no need for change, please let me know. Thank you!

https://go.dev/tour/methods/8

kozlovic commented 1 month ago

This code works perfectly, but I suggested it because A Tour of Go advises against mixing them. If you think there is no need for change, please let me know. Thank you! https://go.dev/tour/methods/8

The "Tour of Go" says that in general we should not mix for a given type, but I don't think that there is any explanation as to why it would be a problem the way it is used here for flags. As you understand, we need the pointer receiver for the set/clear since we need to change the underlying value. But for isSet(), which is checking against the value, we don't need it.

That being said, I don't think having the pointer receiver would have a negative impact (I did a quick bench and don't see a difference), so I would be ok changing it, but I will defer to @derekcollison and @neilalexander that decision.

(PS: I wrote the original func (cf clientFlag) isSet(c clientFlag) bool 8 years ago, from which the readChacheFlag was likely derived from, so if we decide to change the later, we could then also change the former).

neilalexander commented 1 month ago

Thanks for the PR but given all else is equal, I think the current approach is clearer, in that it is obvious from the function definition that a value copy takes place up front and that there is no later mutation of the original value. This is quite a common pattern where a type is an alias of a primitive/basic type.