Fix several bugs (including crashes, and invalid permutations) #2766
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fingolfin
added
kind: bug
fingolfin
force-pushed
the
mh/tests
branch
2 times, most recently
from
ddedacd to
b796525
Compare
Codecov Report
@@ Coverage Diff @@
## master #2766 +/- ##
==========================================
+ Coverage 75.81% 75.87% +0.06%
==========================================
Files 481 481
Lines 241520 241325 -195
==========================================
+ Hits 183104 183113 +9
+ Misses 58416 58212 -204
|
Member
|
@fingolfin Travis currently fails - this should be fixed by #2768, let's give that PR a fast-track. |
The modified functions are guaranteed to be only called if coding is in effect. Make the code reflect that. Also update an outdated message in the manual.
In 1999, the interpreter was fixed to reject "permutations" like (1,2,3,2); but the same fix was not applied to the coder and compiler code. So fix that, and also remove some other differences between the three copies of code that crept in over time. Ideally, this shared code would be factored out into a separate function, but that's not completely trivial, as in one case we iterate over expressions, in one over a GAP list, and in one over a stack of objects.
Specifically, `posobj!{indices}` was meant to relate to `list{indices}` the
same way as `posobj![i]` does to `list[i]`. But it was never fully
implemented. Specifically, no printing or execution functions for the
relevant statement and expression types were implemented; and for the
interpreter and compiler, mostly trying to use these lead to an error a long
the line of "sorry, this feature has not been implemented". Due to all this,
trying to use this feature in a coded function could trigger all kinds of
problems, up to crashes.
Since this code serves no clear purpose, we just remote it, instead of trying
to implement the missing bits.
Fixes gap-system#2765
ChrisJefferson
approved these changes
Sep 4, 2018
Contributor
ChrisJefferson
left a comment
ChrisJefferson
left a comment
There was a problem hiding this comment.
Good fixes all around.
I'm quite upset (not at anyone, just in general), that we've obviously had the bugs which let invalid permutations be created, so well done for noticing!
fingolfin
changed the title
fingolfin
added
release notes: added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
foo!{[1,2,3]}syntax #2765IsBound(dvar)(probably the most obscure of the three bugs)