Follow-up from "Add forced sorting to indexed VP, FT and UT hit cluster containers"
The following discussion from !4595 (merged) should be addressed:
-
@graven started a discussion: (+2 comments)
Note that removing
setOffsets
here, and delaying it until the caller is done with filling, and explicitly forcing the caller to invokeforceSort
is a change in behavior -- all callers will have to be verified to do the right thing (which of course is done as part of this MR), but in addition, any future callers will have to be aware of the fact that they must manually callforceSort
at the right time. (i.e. there is a bit of a booby-trap here waiting to go off).So there should at least be a big comment documenting this, and ideally the code modified so that there is an explicit (and automatic) 'phase change' which delineates the 'this container is being filled, and as of right now one cannot access elements' state and 'the container is now sorted, and one cannot add any elements anymore' state -- i.e. ideally there would be some write-only 'wrapper' which is mandatory in order to call
emplace_back
, and doesn't give access to elements, except for one function which would move the wrapped container out of the wrapper (at which point it also refuses further write operations), at which pointforceSort
would be called just prior to handing over the container, at which point theemplace_back
functions would no longer be available. Basically, it should be impossible to do the wrong thing.But of course that is more work, doesn't make one bit of difference as long as the code is used correctly, and hence probably won't get done. Until at some point in the future someone uses the code incorrectly without realizing, and wastes more time than it would have writing the wrapper which would have made it impossible to do the wrong thing -- or perhaps no-one will ever do the wrong thing, as everyone has either become an expert, or otherwise is too afraid to make any changes.
...
In that case, let's be practical, and split the work to be done in two parts:
- make it work with minimal changes
- clean up the API so that it doesn't need lots of documentation / small print to be used properly, i.e. it is inherently safe to be used (specifically: if the code creating and using the container compiles, then the container is guaranteed to be in a valid state, and ready to be used) The first part is something for 2024-patches (ie. this MR), and the second part is a seperate (later) MR for master (and on top of the first one).