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

Make ScratchMemorySpace semi-regular #3309

Merged
merged 6 commits into from
Sep 5, 2020

Conversation

masterleinad
Copy link
Contributor

@masterleinad masterleinad commented Aug 25, 2020

edited Making ScratchMemorySpace easier to use and to reason about since could not think of any good reason not to do so. The copy-assignment was intentionally deleted in the past but there was no comment explaining that choice.


These seem to not be defined.
Since there is another constructor deleting the private default constructor doesn't seem to be any effect. I could be convinced that the copy constructor should be deleted, though.

Don't use the pre-C++11 way to delete functions but use the rule of 5 instead and also allow move assignment/move construction.

@dalg24
Copy link
Member

dalg24 commented Aug 25, 2020

Of course they are not defined. That's the old way to delete them.
What is the rational to revisit?

@masterleinad
Copy link
Contributor Author

I was trying to understand the Travis problems with #3306 and was stumbling over this class.

@dalg24
Copy link
Member

dalg24 commented Aug 25, 2020

Why did you close? The issue with your PR is not the changes you propose per se, but how you present them.
You are correct that the default constructor is implicitly deleted because we define a constructor.
But you do not discuss the changes to the assignment operator and nothing neither in the title line nor in your description highlight that change.

@masterleinad masterleinad reopened this Aug 26, 2020
Comment on lines -81 to -82
ScratchMemorySpace();
ScratchMemorySpace& operator=(const ScratchMemorySpace&);

Choose a reason for hiding this comment

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

  ScratchMemorySpace() = delete;
  ScratchMemorySpace& operator=(const ScratchMemorySpace&) = delete;

Copy link
Contributor

@nliber nliber left a comment

Choose a reason for hiding this comment

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

This is saying we want ScratchMemorySpace to be copy constructible but not copy assignable. Is that correct?

I'd much prefer the Rule of 5 here, which also has the advantage of not needing to explicitly delete the default constructor.

@masterleinad masterleinad changed the title Remove undefined member functions in ScratchMemorySpace Define all special member functions for ScratchMemorySpace Aug 28, 2020
@masterleinad
Copy link
Contributor Author

This is saying we want ScratchMemorySpace to be copy constructible but not copy assignable. Is that correct?

Yes, we need it to be copy constructible for TeamMembers to be copy constructible.
I was trying to bring up the question if it makes sense to also make it copy assignable (since all the member functions are const anyway) but there is currently really no need.
Let's just keep the current behavior (and allow move assignment/move construction) following the rule of 5 as well.

@nliber
Copy link
Contributor

nliber commented Sep 1, 2020

Given that all the member variables are either int or char*, if we have copy assignment, it would do the exact same thing as move assignment. I'd rather see that and use the rule of 0.

@masterleinad
Copy link
Contributor Author

Since that's what I originally proposed, that's fine with me. 🙂

nliber and others added 2 commits September 3, 2020 23:25
any calls on a default constructed ScratchMemorySpace get_shmem* don't invoke ub
Initialize member variables for default constructed ScratchMemorySpace
@Rombur
Copy link
Member

Rombur commented Sep 4, 2020

You need to apply clang-format

@masterleinad
Copy link
Contributor Author

With the latest changes, ScratchMemorySpace is semiregular now.

@dalg24 dalg24 changed the title Define all special member functions for ScratchMemorySpace Make ScratchMemorySpace semi-regular Sep 4, 2020
@crtrott crtrott merged commit 0599a64 into kokkos:develop Sep 5, 2020
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.

None yet

6 participants