Hello, > I suggest to carefully consider whether to return a "SimpleEntry" (and prominently note that any sort of concurrent modification is a caller responsibility).
I see what you mean and I think that would be a good idea. However, the sticking point is that the 1D implementations for both Euclidean and spherical space internally use the JDK's TreeMap class to store entries, due to its superior performance when compared to the AbstractBucketPointMap class used for other dimensions. TreeMap returns immutable Map.Entry instances from its entry-access methods (e.g., ceilingEntry, floorEntry), so there is not a straightforward way for us to implement the behavior you propose for these dimensions. The options I see are: 1. Have all returned entries be immutable (the current behavior). 2. Return specialized Map.Entry implementations for the 1D cases that call the "put" method on the underlying map when "setValue" is called. Option #2 seems less than ideal so unless there is another approach that I'm missing, I vote for #1. Regards, Matt On Wed, Mar 16, 2022 at 9:48 AM Gilles Sadowski <gillese...@gmail.com> wrote: > > Hi. > > Le mer. 16 mars 2022 à 03:17, Matt Juntunen > <matt.a.juntu...@gmail.com> a écrit : > > > > Hello, > > > > I've made the following changes to the PR: > > - removed the "resolveKey" method from PointMap > > - renamed PointMap.resolveEntry to PointMap.getEntry and > > PointSet.resolve to PointSet.get > > - added an entry on PointMap/PointSet to the user guide > > - addressed Github comments (thanks, Bruno!) > > > > I ran some performance tests regarding the immutable entry instance > > created in the PointMap.getEntry method and there seems to be no > > impact. > > > > > Furthermore, what is actually meant here by "immutable > > instance" (since the "value" could be mutable without the > > map being aware of the fact)? > > > > It is immutable in that the object reference used as the entry value > > cannot be changed. This reference could point to a mutable object. > > This is the same behavior as other Map implementations. > > I don't see that "reference immutability" is mandated by the > "Map" interface (see e.g. [1]). > > I've noted many times that I generally favour (true) immutability: > It makes much sense for "small" data-structures (e.g. for future > potential optimizations[2]). > > However, the "cloud of points" data-structure is at the opposite > of the spectrum from this POV: It is intended to contain a large > number of points whose "key" should indeed be (truly) immutable > but whose value would likely need to be mutable for many actual > use cases. > If a "SimpleImmutableEntry" is returned, then in order to modify > the map's "value" contents, one has to (IIUC) > * retrieve the entry, > * create a new value, > * call "put" (on the map) > rather than > * retrieve the entry > * call "setValue" (on the entry). > So we have a somewhat crippled API that does not bring any > advantage since reference immutability doesn't provide any > security to the map's user (any other caller who is being passed > the same map, is able to change its contents anyways). > > I suggest to carefully consider whether to return a "SimpleEntry" > (and prominently note that any sort of concurrent modification is > a caller responsibility). > > Regards, > Gilles > > [1] > https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#entrySet-- > [2] https://cr.openjdk.java.net/~briangoetz/valhalla/sov/02-object-model.html > > >>> [...] > > --------------------------------------------------------------------- > 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
