search

bluefireteam / ordered_set

A simple implementation for an ordered set
MIT License
20 stars 4 forks source link

refactor: query() should return an Iterable, not List #31

Closed filiph closed 4 months ago

filiph commented 4 months ago

As discussed on Discord, returning a List from QueryableOrderedSet.query() leads to bugs. The developer assumes they own the returned list, then modify it in place (with removeWhere(), for example) to avoid creating yet another iterable. Unbeknownst to the, they're modifying the internal cache of the QueryableOrderedSet.

We agreed to make the breaking change of returning Iterable<C> on this line. Nothing other than that function needs to change, as List is already an Iterable, so we're only exposing the same data structure as a different static type.

This is breaking because people can have the results of query() assigned to typed variables, such as List<Entity> blah = world.query<Entity>();. But that's easy to fix.

If anyone is actually using the return of query() as a modifiable List (as in, they modify it), then that's likely a bug. The change will uncover the bug, so all's good.

The only other valid thing that this could change is code such as var something = blah[index], or for (int i = 0; i < blah.length; i++) doThing(blah[i]);. These are probably rare but valid.

cc @spydon

spydon commented 4 months ago

Sounds good, do you want to open a PR with the change? :)