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

cleanup: resolve compiler warnings #13236

Merged
merged 8 commits into from Feb 8, 2017

Conversation

Projects
None yet
4 participants
@adamemerson
Copy link
Contributor

adamemerson commented Feb 2, 2017

There are 2×10¹⁴ metres of DNA in all the cells of my body. If every run of four base-pairs encoded the word 'hate'…okay that's a bit overboard. Sure, I don't like warnings, but there are a lot of things that deserve to have hate lavished upon them more than compiler diagnostics do.

@adamemerson adamemerson requested a review from tchaikov Feb 3, 2017

@@ -17,7 +17,7 @@ class ExclusiveLock : public ManagedLock<ImageCtxT> {
}

ExclusiveLock(ImageCtxT &image_ctx);

~ExclusiveLock() = default;

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 3, 2017

Contributor

this is not necessary, right? as LeaderLock will have a virtual default dtor anyway. and so does its child classes if any. we can even mark LeaderLock final or mark this method virtual also (and probably override) as well if we really want to be more explicit.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 3, 2017

Contributor

@dillaman do you mind taking a look?

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 3, 2017

Contributor

Nit: base class [1] has a virtual destructor, so by definition this already has a virtual destructor

[1] https://github.com/adamemerson/ceph/blob/1dc449ac5d8ed087af677b61d6da94113bbdf7e4/src/librbd/ManagedLock.h#L47

@@ -98,6 +98,8 @@ class LeaderWatcher : protected librbd::Watcher {
watcher(watcher) {
}

virtual ~LeaderLock() = default;

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 3, 2017

Contributor

ditto.

This comment has been minimized.

Copy link
@adamemerson

adamemerson Feb 3, 2017

Author Contributor

@tchaikov The default destructor is never generated as virtual. If you want a virtual one you have to specify it. (And the compiler was complaining about it not being virtual.)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 3, 2017

Contributor

@adamemerson LeaderLock derives from librbd::ManagedLock<ImageCtxT>, and the latter does have a virtual dtor -- you fixed the full specialization of ManagedLock<MockExclusiveLockImageCtx> in this PR. so LeaderLock will have a virtual dtor even if we don't put

virtual ~LeaderLock() = default;

here, am i right?

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 3, 2017

Contributor

Nit: base class [1] has a virtual destructor, so by definition this already has a virtual destructor

[1] https://github.com/adamemerson/ceph/blob/1dc449ac5d8ed087af677b61d6da94113bbdf7e4/src/librbd/ManagedLock.h#L47

This comment has been minimized.

@tchaikov tchaikov added the cleanup label Feb 3, 2017

@tchaikov tchaikov requested a review from dillaman Feb 3, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Feb 3, 2017

modulo the nit, lgtm.

@tchaikov tchaikov added the needs-qa label Feb 3, 2017

@@ -17,7 +17,7 @@ class ExclusiveLock : public ManagedLock<ImageCtxT> {
}

ExclusiveLock(ImageCtxT &image_ctx);

~ExclusiveLock() = default;

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 3, 2017

Contributor

Nit: base class [1] has a virtual destructor, so by definition this already has a virtual destructor

[1] https://github.com/adamemerson/ceph/blob/1dc449ac5d8ed087af677b61d6da94113bbdf7e4/src/librbd/ManagedLock.h#L47

@@ -98,6 +98,8 @@ class LeaderWatcher : protected librbd::Watcher {
watcher(watcher) {
}

virtual ~LeaderLock() = default;

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 3, 2017

Contributor

Nit: base class [1] has a virtual destructor, so by definition this already has a virtual destructor

[1] https://github.com/adamemerson/ceph/blob/1dc449ac5d8ed087af677b61d6da94113bbdf7e4/src/librbd/ManagedLock.h#L47

@@ -98,6 +98,8 @@ class LeaderWatcher : protected librbd::Watcher {
watcher(watcher) {
}

virtual ~LeaderLock() = default;

This comment has been minimized.

@dillaman dillaman changed the title Hate. Let me tell you how much I've come to hate warnings since I began to live cleanup: resolve compiler warnings Feb 3, 2017

@adamemerson adamemerson force-pushed the adamemerson:wip-alarum branch from 1dc449a to cd93721 Feb 3, 2017

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Feb 3, 2017

@dillaman I have changed it to add virtual destructors to exactly and only the base classes.

adamemerson added some commits Feb 2, 2017

key_value_store: Stop using deprecated omap_get_* functions
This gets rid of deprecation warnings.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
rbd: Fix sign comparison warning
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
rgw: Fix rgw_format construction order
Since we have only one constructor, just use default initialization for
every member that doesn't depend on an argument.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
rgw: Use non-deprecated get_omap function
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
bluestore: Fix redeclaration of struct
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
crush: Do not defeat RVO with unnecessary std::move
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
rgw: Classes with virtual functions get virtual destructors
Including template specializations.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
[librados,libcephfs] Avoid redefinition of rados_t
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

@adamemerson adamemerson force-pushed the adamemerson:wip-alarum branch from cd93721 to bd3bb70 Feb 3, 2017

@tchaikov tchaikov self-assigned this Feb 7, 2017

@tchaikov tchaikov merged commit ec55b39 into ceph:master Feb 8, 2017

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
@tchaikov

This comment has been minimized.

@adamemerson adamemerson deleted the adamemerson:wip-alarum branch May 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.