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

Rename all instances of words master and slave #1253

Merged
merged 36 commits into from Apr 27, 2022

Conversation

IshaanDesai
Copy link
Member

@IshaanDesai IshaanDesai commented Apr 21, 2022

Main changes of this PR

Renaming all instances of words master and slave to primary and secondary or similar to this. Changes to names of variables are done according to the relevance of their use and the functionality of the class. This PR also renamer the words Master and Slave (with capital letters) in most places. The file MasterSlave.cpp and its use in other files is not changed as this would mask the changes done inside the file itself. The file name change is handled in: #1255

Motivation and additional information

We need to do this because we are in the 21st century ✊

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?

@IshaanDesai IshaanDesai linked an issue Apr 21, 2022 that may be closed by this pull request
@IshaanDesai IshaanDesai changed the title Rename master slave Rename all instances of words *master* and *slave* Apr 21, 2022
@IshaanDesai IshaanDesai changed the title Rename all instances of words *master* and *slave* Rename all instances of words master and slave Apr 21, 2022
@IshaanDesai IshaanDesai added this to the Version 2.4.0 milestone Apr 21, 2022
@IshaanDesai IshaanDesai self-assigned this Apr 21, 2022
@IshaanDesai IshaanDesai marked this pull request as ready for review April 21, 2022 11:13
@fsimonis fsimonis requested review from uekerman and removed request for fsimonis April 25, 2022 07:14
@fsimonis fsimonis requested review from fsimonis and removed request for fsimonis April 25, 2022 07:16
Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

Still some work to do here 😁
Simple brute-force replacements do not work here as IMO "pimary" and "secondary" should only be used as adjectives (at least in comments).

Some common suggestions:

  • "to the primary ..." -> "to the primary rank ..."
  • "secondaries" -> "secondary ranks"
  • "primary-primary com" -> "primary com"
  • "master-slave mode": as the server mode does no longer exist, we can just drop these
  • "on all slaves" or similar -> "on all ranks" as typically this also happens on the (previous) master rank

I hope I caught all occasions. See also the suggestions here: #1026 (comment)

In which order do you plan to merge this PR and #1255 ?
You will get nasty merge conflicts. It could be better start #1255 from scratch again.

src/acceleration/BaseQNAcceleration.cpp Outdated Show resolved Hide resolved
src/acceleration/BaseQNAcceleration.cpp Outdated Show resolved Hide resolved
src/acceleration/BaseQNAcceleration.cpp Outdated Show resolved Hide resolved
src/acceleration/BaseQNAcceleration.cpp Outdated Show resolved Hide resolved
src/acceleration/BaseQNAcceleration.hpp Outdated Show resolved Hide resolved
src/utils/MasterSlave.hpp Outdated Show resolved Hide resolved
src/utils/MasterSlave.hpp Outdated Show resolved Hide resolved
src/utils/Parallel.cpp Outdated Show resolved Hide resolved
src/utils/tests/MasterSlaveTest.cpp Outdated Show resolved Hide resolved
src/utils/tests/MasterSlaveTest.cpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

Did not go through everything again. Still open TODOs. Sorry 😁
I would advice against any "find and replace" here. Then, we end up with names like acceptSecondaryRanksConnection where acceptSecondaryConnections feels much more natural.

src/precice/impl/SolverInterfaceImpl.cpp Outdated Show resolved Hide resolved
src/precice/impl/SolverInterfaceImpl.cpp Outdated Show resolved Hide resolved
src/precice/impl/SolverInterfaceImpl.cpp Outdated Show resolved Hide resolved
src/precice/impl/SolverInterfaceImpl.cpp Outdated Show resolved Hide resolved
src/precice/impl/Participant.cpp Outdated Show resolved Hide resolved
src/m2n/BoundM2N.cpp Outdated Show resolved Hide resolved
src/m2n/BoundM2N.cpp Outdated Show resolved Hide resolved
src/m2n/M2N.cpp Outdated Show resolved Hide resolved
src/m2n/M2N.cpp Outdated Show resolved Hide resolved
src/m2n/M2N.cpp Outdated Show resolved Hide resolved
Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

Did look through the details again, but I have a good feeling.

src/com/MPISinglePortsCommunication.hpp Outdated Show resolved Hide resolved
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.

Rename MasterSlave to something more appropriate
3 participants