-
Notifications
You must be signed in to change notification settings - Fork 51
mat64: remove stale TODO #353
Conversation
For my better understanding: why is it stale? |
I need to think more about this. |
OK. Done thinking. It's clear that we have decided that Clone is something that does not preserve the receiver's state in any way (for better or worse). The original design in my head when I started the shadowing code was to engineer safe work places when there was shadowing. On discussion with @btracey we decided that restriction was more sensible. The TODO here was written when overlap detection would have allowed Clone to work somewhere else in case of an overlap. That can't happen anymore. I'll add some extra documentation to Cloner and Dense.Clone, and I also will add the *Vector path. |
af65cc5
to
a2fec3a
Compare
} | ||
case *Vector: | ||
amat := aU.mat | ||
mat.Data = make([]float64, aU.n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more question: the default case reuses the slice because there is no way (unlike with *Vector or RawMatrixer) that such matrix could shadow a Dense, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually could if someone constructed it that way. A matrix that stores its elements in a blas64.General but does not have a RawMatrix method would allow this. Probably I should allocate a new slice there since I am now saying that Clone will not cause shadowing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable and consistent, and reduces my doubt.
This is now covered in the package level documentation and addressed in code.
LGTM |
@btracey / @vladimir-ch Please take a look.