Avoid new type readonly (was: #142) #291
Conversation
After the cleanup of ZmodnZ in gap-system#147, this is the patch that remains.
|
This patch, just like #142, changes the semantics of the type code, and I am not yet convinced this is safe. My questions on #142 were not addressed, and I am opposed to this being merged before somebody at least acknowledges what I pointed out. Namely, if a type is cached, then with this patch, we return the cached type, WITHOUT verifying that its extended data entries match the "parent" data we pass in. I have not tried to actually do it, but it seems conceivable to me that one can artificially create a setup were this leads to a change in behavior with and without this patch applied. But perhaps I am wrong, and this cannot occur, for reasons I missed. Or perhaps it can occur, but we somehow know this never occurs in "real world" scenarios. But I still feel that this then should be explicitly explained. |
|
I agree with @fingolfin that something needs to be considered here, but I don't understand GAP's type system quite well enough to figure out what is going on. It seems to me if there is an issue with the parent type getting out of date, the same problem would occur (perhaps) just generally, once you have made a type with a particular parent, and then the parent gets updated later -- but that might be a different type of issue. |
|
I just tried checking if the parents do always turn out to be the same, and they don't. The sensible thing to do (under the problem that I don't 100% know what's going on) is to add a parent check to the caching. This slightly increases cache misses (from 54 to 67). |
|
Now in #295 |
|
I believe this PR should be closed in favor of my PE #326. Thoughts? |
|
This is obsoleted by #326 |
This is a remake of #142 in which I deleted all commented out code and just return the result of NEW_TYPE in places.
The original PR has a comment about making the codepath (for NEW_TYPE I presume) even simpler. I am now looking at this too. I do suggest to merge this PR though and to close #142, because as far as I can see it already improves the situation.