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

Standardise factories #1842

Merged
merged 39 commits into from
Nov 22, 2019
Merged

Standardise factories #1842

merged 39 commits into from
Nov 22, 2019

Conversation

ZedThree
Copy link
Member

Sorry, this is a horrible PR to drop on a Friday afternoon! On the plus side, it's net negative lines!

Standardises almost all of the various type factories, with the exception of the boundaries and DataFormat, both of which require a fair bit of special handling, not to mention I'm hoping we'll get to rewrite them completely in the near future.

All type factories now return std::unique_ptr<BaseType>, with the exception of Mesh::create, because making bout::globals::mesh be a unique_ptr would break more things than I'm willing to fix just yet.

Standardising all the factories like this means it's now possible to register new anything at runtime. This should allow users to experiment with their own Solver, Laplacian or even Mesh, without needing to recompile the framework.

I've provided bin/bout-v5-factory-upgrader.py which takes a list of files and provides fixes to use the new APIs. It spits out diffs and gives the option to fix each file automatically. It's possible to also dump the output to a patch file, or just shut up and fix everything.

Also adds command line options to just print out a list of available options for various factories. Useful to see what Solvers or Laplacians are available. Less usefully, you can also see what Mesh is available (it's "bout").

Allows customisation of StandardFactory without lots of boilerplate
We need to include the derived-class headers in some "used" source
file so that the linker doesn't remove the "unused" registration
names. Given that the factories are now so short that they mostly
don't need .cxx files, this removes the need for the silly
getDefaultType being a function rather than constexpr value
Also:
- renamed to InvertParFactory to be in line with other factories
- moved into invert_parderiv header
Not a particularly elegant solution, but ensureRegistered can be
defined by derived factories if the base class implementation file is
small/empty. Even having even an empty function in the .cxx file
causes the linker to keep all the symbols in that TU.

Calling ensureRegistered in the factory getInstance means
everything (should) get registered before the first time it is required.
* next: (46 commits)
  Fixed assignment operators in PETSc interface
  Update googletest and mpark.variant submodules
  Applied clang-format to changes made in petsc_interface branch
  Made initialiseTest() method public in PETSc indexer unit tests
  Fixed bug setting yup/ydown values in PetscMatrix
  Added new regions to mesh for use in PETSc interface
  Automatied initialising of GlobalIndexer objects in PETSc interface
  Added PETSc interface method to permit partial assembly of matrices
  Small changes and refactoring to PETSc interface
  Fix typo in initialisation of rgn_outer_x Use lastX() check instead of firstX() when initialising rgn_outer_x
  Applied clang-format to changes made on the petsc_interface branch
  Applied clang-format to the new files added for the PETSc interface
  Eliminated some data races from PETSc interface tests
  Tidied up casting variable to BoutReal and declaration of for loops
  Modified PETSc interface to be thread-safe
  Added PETSc interface files to CMakeLists.txt
  PETSc interface out-of-bounds tests only run for CHECKLEVEL >= 1
  Moved include of petscconf.h to within #ifdef BOUT_HAS_PETSC
  Made some PETSc interface tests conditional on using debug version
  Improved tracking of PetscMatrix::Element.value in += operator
  ...
@ZedThree ZedThree added the breaking change Introduces a change that is not backward compatible with the previous major version label Nov 15, 2019
@ZedThree ZedThree added this to the BOUT-5.0 milestone Nov 15, 2019
* next:
  Add invert_laplace removal to list of breaking changes
  Use new Options API in Laplacian ctor
  Remove deprecated Laplacian APIs
  Remove old invert_laplace interface from test-laplace
  Replace gyroPade* overloads with default arguments
  Remove deprecated overloads of gyro-average operators
@ZedThree
Copy link
Member Author

I was also tempted to move all the factory classes into the corresponding base type and call them Factory. Then for a given type, you could always use Type::Factory. Might be much of a muchness though.

Copy link
Contributor

@bendudson bendudson left a comment

Choose a reason for hiding this comment

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

Thanks Peter! Looks good. Is there documentation on adding implementations anywhere?

@ZedThree
Copy link
Member Author

There is some documentation in the docstring for the Factory, including an example. I figure we won't be making new factories very often, but happy to stick a page in the RTD manual too.

@ZedThree ZedThree merged commit 36c34c1 into next Nov 22, 2019
@ZedThree ZedThree deleted the standardise-factories branch November 22, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Introduces a change that is not backward compatible with the previous major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants