Skip to content

Fix subclass overrides and initialization order - #309

Merged
james-d-mitchell merged 6 commits into
libsemigroups:v1from
Joseph-Edwards:fix-subclass-overrides
Aug 12, 2025
Merged

Fix subclass overrides and initialization order#309
james-d-mitchell merged 6 commits into
libsemigroups:v1from
Joseph-Edwards:fix-subclass-overrides

Conversation

@Joseph-Edwards

@Joseph-Edwards Joseph-Edwards commented Aug 1, 2025

Copy link
Copy Markdown
Collaborator

This PR does two things:

  1. Refactor the sims binding code so that subclasses do not incorrectly override (rather than overload) certain functions (addressing Some classes incorrectly attempt to overload functions in their base class #305) ; and
  2. Change the order in which things are initialized. This reduces the amount of C++ style code that appears in the error messages, amongst other places.

Comment thread src/sims.cpp
Comment on lines +565 to +571
// TODO(0): Uncomment or remove
// thing.def(py::init<Thing const&>(),
// fmt::format(R"pbdoc(
// Construct from a {0} object.
// )pbdoc",
// doc_type)
// .c_str());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Presently, because of how sims.py is setup, the only 1-parameter constructor for a Sims object takes a presentation. However, in libsemigroups we can construct from other Sims objects. Do we want to be able to do that here too?

If yes, we should uncomment the above function and adjust sims.p. If no, we should delete the above function.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Note that this function hasn't always been here. I wrote it for this PR, but then commented it out when I saw how sims.py was setup.

@james-d-mitchell
james-d-mitchell merged commit 1244079 into libsemigroups:v1 Aug 12, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants