Avoid new type readonly2 - DO NOT MERGE! #142
Conversation
|
The travis error looks like just a timeout. |
|
To me too. I've restarted the job to see if this is reproducible. |
|
This PR passed the Travis test from the 2nd attempt. |
|
I strongly recommend to always describe in a PR what it is about, at least briefly -- not everybody knows what "Cleaned up version of previous" refers to... In this case simply mentioning that it is related to #125 would probably be enough :). |
|
Good thing is that this does not segfault anymore for me :-). But I am still somewhat puzzled by this, let me see if I understand things correctly... The old code for e.g. Subtype2 did this, roughly, in case of types that are already in the cache:
The new code does this:
So, I gather this means we implcily rely on the parent data being identical across calls, right? Of course the old code kind of did that already, too, as it modified the cached type object, which would break things unless the extra data stays identical. Hence, this PR is probably safe. Correct? Though we could be paranoid and add a check that verifies the old and new parent data matches, I guess (this causes a slowdown, though, hence we'd only want to do this in a special "debug" mode... perhaps even only do it long enough to run the full test suite to look for failues, and if that works, remove those checks again). |
|
Another remark: With this patch, |
| # fi; | ||
| # od; | ||
| # MakeReadOnlyObj(new); | ||
| # ASS_GVAR(_NEW_TYPE_READONLY, save_flag); | ||
| return new; | ||
| end ); | ||
|
|
||
|
|
||
| BIND_GLOBAL( "Subtype3", function ( type, filter, data ) | ||
| local new, i, save_flag; |
There was a problem hiding this comment.
Note that i and save_flag are unused now. And new could also be removed, in favor of directly returning the result of NEW_TYPE. The same holds for Subtype3, SupType2, SupType3
|
One last purely organizational remark: Personally, before merging this, I would also remove the commented out code (we are using a VCS so we don't need to keep commented out code around); and I would also squash this PR into a single commit (or perhaps two: one modifying the GAP code, one regenerating the .c version from this). This results in less confusing history, and there is no need to preserve e.g. "fix typo in previous change" commits. The easiest way to do this is using an interactive rebase, i.e. |
|
Given how confusing this is, and how ugly code on a critical path becomes, maybe the correct solution is just to eliminate all the cases where things are stored in types after the official "type-data". The only example I can find in the library or packages is the ZmodnZ objects, where it is used to find the default type for new elements when doing arithmetic. I can see no reason why that couldn't be stored in the family instead, or in a global cache indexed by n. |
|
Yeah, ZmodnZ is ugly -- it used to store the value n in three places (see PR #147 -- good that you reminded me about this, I wrote that during the GAP days but forgot to open a PR :). The gist of it is that there are only three users of |
|
@stevelinton In PR #147, I got rid of the ZmodnZ case you mention, i.e. an entry at index ZNZ_PURE_TYPE in the ZmodnZ type objects. |
|
@fingolfin that's great. In that case once PR #147 is merged, this PR can be replaced by a much simpler one, plus some documentation updates to make clear that storing anything in the type after the official type data is not allowed. So do not merge this one. |
olexandr-konovalov
changed the title
|
So #147 indeed was merged, and this PR does not directly apply anymore (thus should be rebased). Also, I think the questions I raised above were not really addressed yet. |
|
I believe this PR should be closed in favor of my PE #326. Thoughts? |
|
#326 has been merged, so this one is obsolete now. |
Cleaned up version of previous