core: remove internalization of affinity strings #10904
Conversation
| RTarget: a.RTarget, | ||
| Operand: a.Operand, | ||
| Weight: a.Weight, | ||
| } |
There was a problem hiding this comment.
I'm conflicted about this transition in this PR and the constraints PR. The previous way copes with changes to the structs better and is more common approach in this file; but I like the explicitness here. Would like to know how you view it and whether we should change the rest of Copy functions?
There was a problem hiding this comment.
I think it comes down to which kind of bug(s) do you want to track down in the future. The explicit assignment here implies you'll have a zero-value or a nil-value if you forget to add the new field.
OTOH using the de-reference copy trick creates a shallow copy - which if you add a pointer field and forget to deep copy it, it will now be shared and mutable by either owner.
I my experience NPE's and zero-values have been easier to track down than shared mutable state, which is why I kind of prefer these explicit copies.
Maybe the best thing would actually be to use something like https://pkg.go.dev/github.com/getlantern/deepcopy, or have Go add copy constructors to the language
core: remove internalization of affinity strings
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Basically the same as #10896 but with the Affinity struct.
Since we use reflect.DeepEquals for job comparison, there is
risk of false positives for changes due to a job struct with
memoized vs non-memoized strings.
Closes #10897