Skip to content
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 potential infinite loop or recursion when computing size of infinite cyclic groups #3580

Merged
merged 1 commit into from
Jul 17, 2019

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jul 16, 2019

This method needs to test whether a group element is the identity; but this
can lead to an infinite recursion if the input is an fp group, as then this
equality check may end up trying to compute the group size. This currently
actually happens when all packages are loaded (with only the default set of
filters, another Size method is selected for cyclic fp groups).

We fix this by adding the CanEasilyCompareElements filter to the argument
filters of the method. Strictly speaking, we'd also want to check for
something like CanEasilyComputerElementOrder, but that doesn't exist.

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jul 16, 2019
This method needs to test whether a group element is the identity; but this
can lead to an infinite recursion if the input is an fp group, as then this
equality check may end up trying to compute the group size. This currently
actually happens when all packages are loaded (with only the default set of
filters, another Size method is selected for cyclic fp groups).

We fix this by adding the `CanEasilyCompareElements` filter to the argument
filters of the method. Strictly speaking, we'd also want to check for
something like `CanEasilyComputerElementOrder`, but that doesn't exist.
@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #3580 into master will decrease coverage by 0.35%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3580      +/-   ##
==========================================
- Coverage   84.46%    84.1%   -0.36%     
==========================================
  Files         696      647      -49     
  Lines      345127   319567   -25560     
==========================================
- Hits       291497   268779   -22718     
+ Misses      53630    50788    -2842
Impacted Files Coverage Δ
lib/grp.gi 87.15% <100%> (-1.84%) ⬇️
lib/grpreps.gi 37.36% <0%> (-46.71%) ⬇️
lib/random.g 66.66% <0%> (-30.56%) ⬇️
lib/cache.gi 70.76% <0%> (-24.62%) ⬇️
lib/ffeconway.gi 68.82% <0%> (-17.68%) ⬇️
lib/grpramat.gi 71.21% <0%> (-16.61%) ⬇️
lib/gprd.gi 58.98% <0%> (-15.5%) ⬇️
lib/read2.g 85.71% <0%> (-14.29%) ⬇️
lib/polyconw.gi 65.24% <0%> (-14.19%) ⬇️
lib/read1.g 85.36% <0%> (-13.42%) ⬇️
... and 304 more

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 84.321% when pulling 19f99af on fingolfin:mh/fix-cyclic-size into b5373a3 on gap-system:master.

@fingolfin fingolfin merged commit 2e9ace1 into gap-system:master Jul 17, 2019
@fingolfin fingolfin deleted the mh/fix-cyclic-size branch July 17, 2019 10:27
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 20, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants