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

Fix base class should be explicitly initialized in copy constructor #3076

Merged

Conversation

SunBlack
Copy link
Contributor

@SunBlack SunBlack commented May 8, 2019

No description provided.

@SunBlack SunBlack force-pushed the fix_copy_constructor branch 3 times, most recently from a0a94de to 970679d Compare May 8, 2019 22:53
@taketwo
Copy link
Member

taketwo commented May 9, 2019

Can we close this in favor of #3075 or is there anything that was not addressed there?

@SunBlack
Copy link
Contributor Author

SunBlack commented May 9, 2019

As I touched 18 files and we have only 12 conflicts, there must be something open. I check it later.

@SunBlack
Copy link
Contributor Author

SunBlack commented May 9, 2019

It is mostly a cleanup PR now, even it contains still some warning fixes.

Btw suggestion for the style guide:

  • In case copy & assignment constructor are not needed, the assignment constructor should be deleted (personal preference by me, but I think we should have a guideline there)
  • Deleted functions should be public, because normal you are looking in public member for special members. So it increases readability.

@SergioRAgostinho
Copy link
Member

* In case copy & assignment constructor are not needed, the assignment constructor should be deleted (personal preference by me, but I think we should have a guideline there)

Can you rephrase this sentence revising the terms you used. Just to avoid incorrect interpretation

Class(const Class& other) // copy constructor

Class& operator=(const Class& other) // copy assignment

Something is either an assignment operator or a constructor but never an assignment constructor.

@SunBlack
Copy link
Contributor Author

SunBlack commented May 9, 2019

Sorry, I meant I prefer deleting copy constructor over deleting copy assignment.

@SergioRAgostinho
Copy link
Member

Sorry, I meant I prefer deleting copy constructor over deleting copy assignment.

The general case is the following: if one needs a user defined copy constructor, it also needs a user defined copy assignment. I've never faced a situation in which you need one and not the other. So my suggestion is: either both or none should be defined.

Deleted functions should be public, because normal you are looking in public member for special members. So it increases readability.

Valid point. On this one I personally prefer to not have implicitly deleted methods on the headers. Less code to maintain.

@SunBlack SunBlack force-pushed the fix_copy_constructor branch 4 times, most recently from 069a70e to c38e548 Compare May 24, 2019 16:44
@SunBlack
Copy link
Contributor Author

Had now finally time to fix last compile issues. So it's ready to review :).

@SergioRAgostinho SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Jun 19, 2019
@SergioRAgostinho SergioRAgostinho removed the needs: code review Specify why not closed/merged yet label Jun 21, 2019
@SergioRAgostinho SergioRAgostinho merged commit a3df27b into PointCloudLibrary:master Jun 21, 2019
@SunBlack SunBlack deleted the fix_copy_constructor branch June 21, 2019 17:00
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.

3 participants