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

Allow to copy and move construct ViewPlainPtr #679

Merged

Conversation

BenjaminW3
Copy link
Member

@BenjaminW3 BenjaminW3 commented Oct 10, 2018

This is a part of #655. I will think about the assignment operators separately.

[edit from psychocoder]
PR adds a test for copy and move constructor

@BenjaminW3 BenjaminW3 added this to the Version 0.4.0 milestone Oct 10, 2018
@ax3l ax3l requested review from tdd11235813 and removed request for ax3l October 10, 2018 09:34
@psychocoderHPC psychocoderHPC merged commit 93e5b98 into alpaka-group:develop Oct 11, 2018
@sbastrakov
Copy link
Member

I'm probably missing smth, but why the body of the move constructor is required in this case? It looks to me as it just repeats what the default copy constructor would do, and so acts exactly the same as the deleted move constructor would.

@BenjaminW3 BenjaminW3 deleted the topic-ViewPlainPtr-copy-move branch October 15, 2018 13:04
@BenjaminW3
Copy link
Member Author

Yes, it does exactly the same as the copy-constructor. Moving of this class simply means to copy the pointer and the other members. When the move-constructor is deleted, we would not be able to move the class at all but there is no reason to disallow it, even though it does not have any advantages. The compiler is not able to generate this default move-constructor due to the const members.

@sbastrakov
Copy link
Member

sbastrakov commented Oct 15, 2018

I do not think it is correct. std::move is essentially a glorified typecast, it just enables the right constructor to be called. So deleted (including implicitly, i.e. not generated) move constructor results only in copy constructor being called instead of move, which is the desired behavior here. Example, bottom of page, class C
My point is, there is nothing wrong with the code currently, but the same effect could have been achieved easier and potentially better with respect to future changes to this class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants