-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix argument checking in FreeSemigroup
, prevent creation of inconsistent objects
#4372
Conversation
25f472e
to
c801433
Compare
0e0c8c7
to
a83c443
Compare
See gap-system#1385 It was previously possible to use `FreeSemigroup` to create a seemingly free semigroup with zero generators (rank zero), but this object believed itself to be infinite, and to contain elements (such a semigroup would be empty). Note that free semigroups of rank zero are not documented as being supported. Some other ways that one might want to create free semigroups of rank zero led to unhelpful error messages, and in general, the argument checking of `FreeSemigroup` was not very thorough and gave sometimes unhelpful error messages. In this commit, I overhaul the way that `FreeSemigroup` checks its arguments, in particular always disallowing a free semigroup of rank zero, and overall making the checking more robust, and making the error messages more descriptive. This addresses the _bugs_ reported in issue gap-system#1385, although it does not address the _feature request_ in that issue to support free semigroups of rank zero. I also took the opportunity to fix a typo in the documentation for `FreeSemigroup`, and to make a clarification. Further improvements could be made to this documentation, but I leave that to the future.
a83c443
to
0f122d9
Compare
Just to verify: issue #1385 also mentions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
Correct, this PR doesn't do anything about In my opinion, #1385 is about two things: a wrong-result bug report ( The primary purpose of this PR was to deal with the wrong-result bug in preparation for GAP 4.12.0. Given the way that the issue was written, I assumed that the wrong-result bug only applied I'll have a think about whether I want to do to address this too, and if so how. I don't want this to snowball 🙂! The code for I will probably create a new issue, as a feature request for supporting these things on zero generators – unless it turns out that it makes much more sense to just combine all those changes in this one PR. |
This PR has a "do not merge" label. So, what is left to be done? |
I wanted to decide whether this PR should also deal with |
FreeSemigroup
more thoroughlyFreeSemigroup
, prevent creation of inconsistent objects
See #1385.
It was previously possible to use
FreeSemigroup
to create a seemingly free semigroup with zero generators (rank zero), but this object believed itself to be infinite, and to contain elements (such a semigroup would be empty). Note that free semigroups of rank zero are not documented as being supported.Some other ways that one might want to create free semigroups of rank zero led to unhelpful error messages, and in general, the argument checking of
FreeSemigroup
was not very thorough and gave sometimes unhelpful error messages.In this commit, I overhaul the way that
FreeSemigroup
checks its arguments, in particular always disallowing a free semigroup of rank zero, and overall making the checking more robust, and making the error messages more descriptive.This addresses the bugs reported in issue #1385, although it does not address the feature request in that issue to support free semigroups of rank zero.
I also took the opportunity to fix a typo in the documentation for
FreeSemigroup
, and to make a clarification. Further improvements could be made to this documentation, but I leave that to the future.Description
Text for release notes
Fix bugs caused when calling
FreeSemigroup
with incorrect arguments.(End of text for release notes)