Fix issue 623 #625
Fix issue 623 #625
Conversation
|
I need to add the tests from GAP to this PR too. |
james-d-mitchell
added
gap-compatibility
james-d-mitchell
force-pushed
the
fix-issue-623
branch
2 times, most recently
from
ae76c75
to
de927e1
Compare
james-d-mitchell
force-pushed
the
fix-issue-623
branch
from
de927e1
to
e376c2f
Compare
|
Might eventually resolve Issue #623. Genius idea to rename |
| } | ||
| CHANGED_BAG(res); | ||
| } | ||
| } |
There was a problem hiding this comment.
If you check for the kernel version as suggested above, then you could also remove this function and just call SortPlistByRawObj
| @@ -57,6 +59,126 @@ UInt T_SEMI = 0; | |||
| UInt T_BIPART = 0; | |||
| UInt T_BLOCKS = 0; | |||
|
|
|||
| // Forward decl | |||
There was a problem hiding this comment.
Maybe this?
| // Forward decl | |
| // Forward decl of a function imported from the GAP kernel |
I wonder how the code even loads with this in older GAP versions, though?!? Anyway, with the #if check for kernel versions, this would be a non-issue.
Ah silly me, in older GAP versions, FuncIMAGE_SET_TRANS_INT already exists anyway. And in gap-system/gap#3699 you add this function to a header file. So with the #if approach, you could safely delete this decl here anyway.
There was a problem hiding this comment.
Right you are, deleted!
Co-Authored-By: Max Horn <max@quendi.de>
Co-Authored-By: Max Horn <max@quendi.de>
|
Thanks for your comments @fingolfin, I should have removed the request for you to review until I got this to work, I somehow thought this would be easier than it turned out to be... I'll try to update the PR soon. |
|
@james-d-mitchell please take your time |
Co-Authored-By: Max Horn <max@quendi.de>
|
Ok, I've spent another afternoon trying to figure out how to do this, and not break backwards compatibility for Semigroups, and I'm ready to give up. The problem is to make We could stop supporting older versions of GAP, but doing that just to move this one function from the GAP kernel doesn't seem worth it to me. Another solution would be to remember that we want to do this and do it the next time it makes sense to update the version of GAP required by Semigroups. If you have a better suggestion @fingolfin, I'm happy to hear it. |
|
@james-d-mitchell I am sorry that you wasted so much time on this :-(. Please don't bother doing any more on this. I'll take a look if I can see an "easy" (whatever that means) way to pull this off, and then I'll submit a PR; but otherwise, it really won't hurt us to leave |
|
Not to worry @fingolfin I don't think it was a waste of time, just didn't work out. I'm going to close this PR, since I've given up on this. |
This PR fixes Issue 623 by adding OnPosIntSetsTrans to the kernel module of the Semigroups package.