Skip to content

Refine SetSize and remove some immediate methods#6347

Open
fingolfin wants to merge 1 commit intomasterfrom
mh/removed-immediate-methods
Open

Refine SetSize and remove some immediate methods#6347
fingolfin wants to merge 1 commit intomasterfrom
mh/removed-immediate-methods

Conversation

@fingolfin
Copy link
Copy Markdown
Member

@fingolfin fingolfin commented Apr 28, 2026

The removed methods mostly were already obsolete due to the custom SetSize. Only the "non emptiness" was not recorded right, which the new code in SetSize now handles.

Immediate methods are always a bit problematic because they slow down things. We added the custom SetSize (and SetDimension) precisely to reduce the need for some of these, but it seems we then did not remove all the now obsolete methods.

The removed methods mostly were already obsolete due to the custom
SetSize. Only the "non emptiness" was not recorded right, which
the new code in `SetSize` now handles.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.62%. Comparing base (5fbcf41) to head (3670347).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6347   +/-   ##
=======================================
  Coverage   78.62%   78.62%           
=======================================
  Files         684      684           
  Lines      292701   292691   -10     
  Branches     8686     8660   -26     
=======================================
- Hits       230147   230142    -5     
+ Misses      60744    60741    -3     
+ Partials     1810     1808    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fingolfin fingolfin changed the title Refine SetSize and remove some immediate methods Refine SetSize and remove some immediate methods Apr 28, 2026
@fingolfin fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Apr 28, 2026
Comment thread lib/modfree.gi
Comment on lines -107 to -116
InstallImmediateMethod( IsFinite,
IsFreeLeftModule and HasIsFiniteDimensional, 0,
function( V )
if not IsFiniteDimensional( V ) then
return false;
else
TryNextMethod();
fi;
end );

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removal might be considered somewhat borderline, because it triggers on HasIsFiniteDimensional, not on HasDimension (were our SetDimension already took care). But realistically, the set of code that does SetIsFiniteDimensional(V, false) and also relies on this kind of IsFinite immediate method is hopefully empty. (And besides, why not use SetDimension(V, infinity) instead and solve this and more in one go)

Copy link
Copy Markdown
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
(First I stumbled over the different handling for the filters IsTrivial, IsNonTrivial, but this is fine because of the logical implications that connect them.)

I would say that immediate methods are problematic in particular because one can switch them off. Thus one cannot rely on that they will do what one expects.

@fingolfin fingolfin added this to the GAP 4.16.0 milestone Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants