Hi all, Le 23/02/2012 16:45, Jörg Schaible a écrit : > Gilles Sadowski wrote: > >>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1245061&view=rev >>>>>>>>>> Log: >>>>>>>>>> Removed unneeded clone. >>>>>>>>>> >>>>>>>>>> The clone did not protect the array used, only the reference >>>>>>>>>> ones. JIRA: MATH-650 >>>>>>>>> >>>>>>>>> -1 >>>>>>>>> >>>>>>>>> That was the whole point of the clone - to protect the original >>>>>>>>> external data. >>>>>>>> >>>>>>>> Please (re-)explain what you mean by "protect". Cf. my comment >>>>>>>> on the JIRA page. >>>>>>> >>>>>>> See also your comment of 30/Nov/11 00:31. >>>>>> >>>>>> I know, but my latest comment overrides that one. >>>>>> >>>>>>> The arrays in FastMathLiteralArrays are private, but the access >>>>>>> methods are not, and returning an array allows the caller to modify >>>>>>> array elements. >>>>>> >>>>>> It's true, but unless I'm mistaken, it doesn't matter since in the >>>>>> end there is one and only one array that will be _used_ (the >>>>>> modified one) and having a pristine copy somewhere else will not >>>>>> prevent the dire consequences of using the modified one... ;-/ >>>>> >>>>> Not sure I follow that. >>>>> >>>>> Without clone, the array entries can be changed at any time. >>>>> >>>>> With clone, there are two pristine copies; neither can be changed >>>>> directly as they are stored in private arrays. >>>> >>>> The question is: Why calling "clone"? >>>> The answer is: You get a copy and you cannot change the array stored in >>>> "FastMathLiteralArrays". >>>> But that doesn't save you from the bug that consists in changing an >>>> array entry from within "FastMath". >>>> If that potential bug exists, some methods will produce erroneous >>>> values because they use the copy, not the pristine original array >>>> stored in "FastMathLiteralArrays". [After loading, the original array >>>> is never accessed again.] >>>> >>>> Now if you just obtain a reference to the original array (i.e. no call >>>> to "clone"), the bug will indeed modify it. >>>> But the consequences are not worse than in the above scenario: The >>>> exact same erroneous values will be computed. >>> >>> Not strictly true, because it's not only the FastMath class that can >>> corrupt the array. >> >> Only "our" classes (in the package "o.a.c.m.util") can access (and >> corrupt) the arrays. >> >>> >>> Of course clone does not protect against bugs in FastMath; only array >>> entry getters can do that. >> >> Yes. That's what I said. >> >>> But that's not the point; >> >> Yes, that's the point. >> >>> since the arrays were (at your insistence) >>> moved out of FastMath itself, this necessarily meant exposing the >>> contents to some degree. >>> Using clone closes that potential exposure - as would using array entry >>> getters. >> >> "clone" provides protection against CM developers, which I deem >> unnecessary, since those same developers would also be able to make the >> "mistake" of modifying the contents "FastMathLiteralArrays". [And the same >> mistake could also happen if the arrays were stored inside the "FastMath" >> class.] > > I support Gilles here, the methods are package scoped and therefore > internal. A CM user will have to create an own package with same name (which > will not work for sealed jars or with OSGi bundles anyway) or use reflection > to access them. At that stage he should already know what he does. For CM > itself there's no need to pay the penalty to create a copy.
I don't have any clear cut opinion here, but has said in another thread in this list we do have a veto on a code change, and this cannot be ignored as per Apache rules. So I a going to undo that change and get the clone back. Luc > > - Jörg > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
