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

osd/OSDMap: Uncomment code to enable private default constructors #12597

Merged
merged 1 commit into from Dec 24, 2016

Conversation

Projects
None yet
3 participants
@badone
Contributor

badone commented Dec 21, 2016

C++11 is here so let's enable this code.

Signed-off-by: Brad Hubbard bhubbard@redhat.com

@badone

This comment has been minimized.

Contributor

badone commented Dec 21, 2016

I swear I built and tested this locally with no problems. Looking into this.

@badone badone changed the title from OSDMap: Uncomment code to enable private default constructors to [DNM] OSDMap: Uncomment code to enable private default constructors Dec 21, 2016

@badone badone removed the needs-review label Dec 21, 2016

OSDMap: Uncomment code to enable private default constructors
C++11 is here so let's enable this code.

Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
@badone

This comment has been minimized.

Contributor

badone commented Dec 22, 2016

@liewegas and @athanatos how does this look?

@badone badone added the needs-qa label Dec 22, 2016

@badone badone changed the title from [DNM] OSDMap: Uncomment code to enable private default constructors to OSDMap: Uncomment code to enable private default constructors Dec 22, 2016

@@ -275,12 +275,10 @@ class OSDMap {
}
// no copying
/* oh, how i long for c++11...
private:
OSDMap(const OSDMap& other) = default;

This comment has been minimized.

@tchaikov

tchaikov Dec 22, 2016

Contributor

if we want to disable the default copy ctor and assign operator, we might want to put

OSDMap(const OSDMap& other) = delete;
OSDMap& operator=(const OSDMap& other) = delete;

instead, and probably remove the private: before them also.

This comment has been minimized.

@badone

badone Dec 22, 2016

Contributor

Yes Kefu, I believe the intent was that they would only be usable within the OSDMap class and unusable from outside of that class but I'll let @liewegas and/or @athanatos comment on that. Happy to make the change if required of course and thanks for taking a look.

This comment has been minimized.

@liewegas

liewegas Dec 22, 2016

Member

if everything builds with = delete, let's do that. I forget if there is a private user.

@@ -275,12 +275,10 @@ class OSDMap {
}
// no copying
/* oh, how i long for c++11...
private:
OSDMap(const OSDMap& other) = default;

This comment has been minimized.

@liewegas

liewegas Dec 22, 2016

Member

if everything builds with = delete, let's do that. I forget if there is a private user.

@@ -41,7 +41,7 @@ TYPE_FEATUREFUL(entity_inst_t)
#include "osd/OSDMap.h"
TYPE(osd_info_t)
TYPE(osd_xinfo_t)
TYPE_FEATUREFUL_STRAYDATA(OSDMap)
TYPE_FEATUREFUL_NOCOPY(OSDMap)

This comment has been minimized.

@liewegas

liewegas Dec 22, 2016

Member

make sure test/encoding/readable.sh works with this change...

This comment has been minimized.

@badone

badone Dec 22, 2016

Contributor

Sure, that was my original suggestion as I thought that was your intent originally but Sam talked me out of it :) Another change coming up...

This comment has been minimized.

@badone

badone Dec 22, 2016

Contributor

"= delete" I mean

This comment has been minimized.

@badone

badone Dec 22, 2016

Contributor

Alas, it doesn't build since we use assignment in deepish_copy_from(). Let me know which way you want to jump.

This comment has been minimized.

@badone

badone Dec 22, 2016

Contributor

test/encoding/readable.sh works with the current changes BTW.

@liewegas

This comment has been minimized.

Member

liewegas commented Dec 22, 2016

@badone

This comment has been minimized.

Contributor

badone commented Dec 22, 2016

@tchaikov happy with that?

@liewegas liewegas changed the title from OSDMap: Uncomment code to enable private default constructors to osd/OSDMap: Uncomment code to enable private default constructors Dec 22, 2016

The current code relies on these member fucntions being available so making them private stops other code using them and forces them to use deepish_copy_from()

@badone badone merged commit 4afdefe into ceph:master Dec 24, 2016

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@badone badone deleted the badone:wip-osdmap-nocopy branch Dec 24, 2016

@badone

This comment has been minimized.

Contributor

badone commented Dec 24, 2016

Sorry, I merged this before it passed testing.

@liewegas

This comment has been minimized.

Member

liewegas commented Dec 25, 2016

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