Thank you for the very helpful review.
I see your point about being able to use a FrameDict where a FrameSet is expected possibly having surprising results (inability to add a frame due to domain name conflict). However, I feel the risk of problems is low and does not justify the added code complexity. Conceptually a FrameDict is just a FrameSet that adds optional index-by-domain-name, and the class inheritance does reflect that.
I strongly agree with your other points and I think I have made most your suggested changes. The results are a big improvement in safety, test coverage and (via copydoc) test coverage. See below.
The changes I did not make are:
- I did not remove statements such as using FrameSet::setCurrent because they are required to make the original method visible if the derived class defines a variant. I agree they are ugly.
- I retained two math computations in unit tests, as I prefer to be more direct about the expected results instead of leaving it to FrameSet.
In order to safely keep the dict in synch with the contained frames, for any operation that requires modifying the dict I make a copy of the contents as a FrameSet (which is what the underlying AST object is), modify that, then set the new dict and, if that succeeds, swap the AST objects. This is slower than the old code, but safer. Operations that modify the contained frames are expected to be rare, so I think we can easily live with the extra copying.
I put all the changes on a new commit in the hope that it will make it easier to see what changes I made. I plan to squash it before (eventually) merging.