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

Introduce private SolverInterfaceImpl constructor #1261

Conversation

BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Apr 24, 2022

Main changes of this PR

Introduces a private constructor. Closes #1260.

Author's checklist

  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I ran make format to ensure everything is formatted correctly.
  • I sticked to C++14 features.
  • I sticked to CMake version 3.10.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@BenjaminRodenberg BenjaminRodenberg added bug preCICE does not behave the way we want and we should look into it (and fix it if possible) usability This issue will make preCICE easier for non-expert users labels Apr 24, 2022
@BenjaminRodenberg BenjaminRodenberg added this to the Version 2.4.0 milestone Apr 24, 2022
@BenjaminRodenberg BenjaminRodenberg self-assigned this Apr 24, 2022
@fsimonis fsimonis modified the milestones: Version 2.4.0, Version 3.0.0 Apr 25, 2022
@BenjaminRodenberg
Copy link
Member Author

I'm not sure whether this is actually breaking and must go into 3.0.0 or we could also declare it as a simple bugfix and put it into 2.x.x.

@fsimonis
Copy link
Member

fsimonis commented Jun 13, 2022

This is an ideal fit for an std/boost::optional as it allows differentiating between a user-provided nullptr and no user-provided ptr. This requires an upgrade to C++17 for the std:: version #1201.

It allows you to write:

if (communicator.has_value()) {
  PREICE_CHECK(communicator.value() != nullptr,
    "...");
}

There are 2 ways to implement this cleanly:

1. keep the pimple clean

Have 2 constructors with the same interface in both SolverInterface and SolverInterfaceImpl, then forward the latter two to a single private constructor that takes a ::optional<void *>.

class SolverInterfaceImpl{
public:
  SolverInterfaceImpl(
      std::string        participantName,
      const std::string &configurationFileName,
      int                solverProcessIndex,
      int                solverProcessSize)
: SolverInterfaceImpl(participantName, configurationFileName, solverProcessIndex, solverProcessSize, nullopt) {}

  SolverInterfaceImpl(
      std::string        participantName,
      const std::string &configurationFileName,
      int                solverProcessIndex,
      int                solverProcessSize,
      void *             communicator)
: SolverInterfaceImpl(participantName, configurationFileName, solverProcessIndex, solverProcessSize, make_optional(communicator)) {}
private:
  SolverInterfaceImpl(
      std::string        participantName,
      const std::string &configurationFileName,
      int                solverProcessIndex,
      int                solverProcessSize,
      std::optional<void *> communicator)
}

2. keep the interface clean

Have 1 constructor in SolverInterfaceImpl that takes a ::optional<void *>, then call it from the 2 constructors in SolverInterface.

class SolverInterfaceImpl{
public:
  SolverInterfaceImpl(
      std::string        participantName,
      const std::string &configurationFileName,
      int                solverProcessIndex,
      int                solverProcessSize,
      std::optional<void *> communicator)
}

@BenjaminRodenberg
Copy link
Member Author

This is an ideal fit for an std/boost::optional as it allows differentiating between a user-provided nullptr and no user-provided ptr. This requires an upgrade to C++17 for the std:: version #1201.

As long as we did not conclude on #1201 I'm a bit hesitant implementing it here, because this would make this PR unmergable until we upgrade to C++ 17. The solution sounds reasonable, but does (at the current state) not fit into preCICE (we can still keep it as an issue and improve it in the future).

@fsimonis
Copy link
Member

An optional is really only a slight refactoring of this solution. So, let's merge this once CI passes.

@BenjaminRodenberg BenjaminRodenberg added the breaking change Breaks backwards compatibility and users need to act label Jun 13, 2022
@BenjaminRodenberg
Copy link
Member Author

I just realized that this change is considered breaking. Therefore, we should not merge this PR before we are sure that our next release will be v3.0.0. We can still keep this PR open and consider it ready to merge.

@fsimonis
Copy link
Member

fsimonis commented Jun 15, 2022

The case, which could be regarded as a breaking change, would already break MPI.
So, we don't see this as breaking.

@fsimonis fsimonis removed the breaking change Breaks backwards compatibility and users need to act label Jun 15, 2022
@BenjaminRodenberg
Copy link
Member Author

This PR is ready to merge as soon as the tests are complete.

@BenjaminRodenberg BenjaminRodenberg merged commit 7283b15 into precice:develop Jun 15, 2022
@BenjaminRodenberg BenjaminRodenberg deleted the i-1260-disallow-nullptr-communicator branch June 15, 2022 12:27
@uekerman uekerman modified the milestones: Version 2.x.x, Version 2.5.0 Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug preCICE does not behave the way we want and we should look into it (and fix it if possible) usability This issue will make preCICE easier for non-expert users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SolverInterface constructor accepts communicator = nullptr
3 participants