Fix a dead store in setSPairGroupSize, and delete the unbuilt MESClassicGBAlg - #88
Merged
d-torrance merged 2 commits intoSep 19, 2026
Merged
Conversation
setSPairGroupSize's zero branch assigned mReducer.preferredSetSize() to
groupSize, its own by-value parameter, and returned. The value went
nowhere: the member the algorithm reads is mSPairGroupSize, which the
else branch writes and which that branch never touched.
Nothing was wrong in the output because the constructor at
ClassicGBAlg.cpp:127 already initialises mSPairGroupSize to
reducer.preferredSetSize(), the same value the discarded line computes.
So the branch that looks like it establishes the default was the branch
that did nothing, and the default was right by coincidence. The two
cannot disagree: preferredSetSize is a constant per reducer, 1 for
TypicalReducer and 100000 for F4Reducer, and mReducer is bound to the
same object the constructor asked.
The likely origin is a mis-edit of
if (groupSize == 0)
groupSize = mReducer.preferredSetSize();
mSPairGroupSize = groupSize;
where the assignment became an else branch, which leaves the nonzero
path working and guts the zero path.
That distinction matters to the next person to change how the default is
chosen, since their edit to that line would silently have no effect.
Item 21 of the review, which is about the threading defaults, is exactly
that kind of change.
No behaviour change, and none is testable: ClassicGBAlg is declared
inside its own .cpp with a single caller, so there is no seam to observe
this from. Checked by hand instead, through the value mgb prints as
"S-pair group size", before and after:
-sPairGroupSize 0, default reducer 1 -> 1
-sPairGroupSize 0, -reducer 26 100000 -> 100000
-sPairGroupSize 5 5 -> 5
MESClassicGBAlg.cpp:137 has the same dead store and is left alone: that
file is in neither Makefile.am nor src/CMakeLists.txt, so it compiles
nowhere, and whether it should exist at all is the larger question
recorded under item 22.
Verified: 253/253 in cmake Release and Debug, no build warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MESClassicGBAlg.cpp and .hpp arrived in e6996a3, a bulk backport of 14 files from Macaulay2/M2#2160 "Refactor mathic". They have had no substantive change since, only the repo-wide whitespace pass. They were never built. Both are absent from Makefile.am and src/CMakeLists.txt, including EXTRA_DIST, so no release tarball has ever contained them, and build/vs12's project file omitted them too until PR #78 deleted it. Nothing in the tree refers to them. They cannot be built. MESClassicGBAlg.cpp includes ClassicGBAlg.hpp, not its own header, and defines mgb::ClassicGBAlg -- the same class ClassicGBAlg.cpp defines. Adding it to a build is a duplicate symbol. Its own header declares something different again, mgbF4, which the .cpp never includes. There is nothing in them to keep. The algorithm is identical: computeGrobnerBasis matches outright, and step and insertReducedPoly differ only in spelling MATHICGB_ASSERT as plain assert. Every other difference is a removal -- the statistics block that ClassicGBAlg builds with 51 ColumnPrinter calls is gone, printMemoryUse is an empty body reading "TODO: bring over from mathicgb or rewrite", and LogDomain is dropped. 226 fewer lines, all of them things taken out. What it was is a port in progress: the file strips this project's own macros so that it compiles inside Macaulay2's engine instead, which its compile-command names as $M2BUILDDIR/Macaulay2/e. Macaulay2 does not have it today either -- the name appears nowhere in that repository. Deleting it closes the "in two files" half of review item 22, and removes the second copy of the timing label item 20 fixed and a third site of the ignored queueType parameter in item 27. It also stops every grep for a bug in ClassicGBAlg.cpp returning two answers, one of which compiles nowhere, which is how item 22 came to be written as it was. Recoverable from history if anyone wants it back. Verified: 253/253 in cmake Release and Debug, no build warnings, which is unsurprising since nothing compiled these files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Two independent cleanups in
ClassicGBAlg's neighbourhood, neither changingbehaviour.
1.
setSPairGroupSizeassigned the default to its own parametergroupSizeis the by-value parameter;mSPairGroupSizeis the member thealgorithm reads. So passing 0 — which the CLI documents as "use an appropriate
default" — did nothing at all.
The output was right anyway, because the constructor already initialises
mSPairGroupSizetoreducer.preferredSetSize(), the same value the discardedline computes. The two cannot disagree:
preferredSetSizeis a constant perreducer (1 for
TypicalReducer, 100000 forF4Reducer) andmReducerisbound to the object the constructor asked. So the branch that appears to
establish the default was the branch that did nothing, and the default was
correct by coincidence.
The likely origin is a mis-edit of
where the trailing assignment became an
else, which leaves the nonzero pathworking and guts the zero path.
No behaviour change, and none is observable —
ClassicGBAlgis declared insideits own
.cppand has one caller, so there is no seam for a test. Checked byhand through the value
mgbprints asS-pair group size:-sPairGroupSize 0, default reducer-sPairGroupSize 0 -reducer 26-sPairGroupSize 5It matters to whoever next changes how that default is chosen, since their edit
to that line would silently have no effect.
2.
MESClassicGBAlgis deletedMESClassicGBAlg.cppand.hpparrived ine6996a3, a bulk backport of 14files from Macaulay2/M2#2160 "Refactor mathic", and have had no substantive
change since.
Never built. Absent from
Makefile.amandsrc/CMakeLists.txtincludingEXTRA_DIST, so no release tarball has ever contained them.build/vs12'sproject file omitted them too, until PR #78 deleted it. Nothing in the tree
refers to them.
Cannot be built. The
.cppincludesClassicGBAlg.hpprather than its ownheader and defines
mgb::ClassicGBAlg— the classClassicGBAlg.cppalreadydefines. Adding it to a build is a duplicate symbol. Its own header declares
something else entirely,
mgbF4, which the.cppnever includes.Nothing in it to keep. The algorithm is identical —
computeGrobnerBasismatches outright,
stepandinsertReducedPolydiffer only in spellingMATHICGB_ASSERTas plainassert. Every other difference is a removal: thestatistics block built from 51
ColumnPrintercalls is gone,printMemoryUseis an empty body reading
// TODO: bring over from mathicgb or rewrite, andLogDomainis dropped. 226 fewer lines, all subtractions.It was a port in progress — stripping this project's macros so the file would
compile inside Macaulay2's engine, which its own
compile-commandnames as$M2BUILDDIR/Macaulay2/e. Macaulay2 does not have it today either; the nameappears nowhere in that repository.
Deleting it also removes a second, untouched copy of the mislabelled timing
line PR #87 fixed, and a third site of an ignored
queueTypeparameter.Mostly it stops every grep for a bug in
ClassicGBAlg.cppreturning twoanswers, one of which compiles nowhere. Recoverable from history.
Verification
commit.
S-pair group sizetable above, measured before and after.MESClassic, tomgbF4, or to its include guard survives inany tracked file.
No autotools
distcheckrun locally; neither commit touches a build system andCI covers it.
🤖 Generated with Claude Code