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

rbd: add override in rbd subsystem #13437

Merged
merged 1 commit into from Feb 17, 2017

Conversation

Projects
None yet
4 participants
@Liuchang0812
Copy link
Contributor

Liuchang0812 commented Feb 15, 2017

@dillaman dillaman self-assigned this Feb 15, 2017

@@ -59,7 +59,7 @@ class ThreadPoolSingleton : public ThreadPool {
"rbd_op_threads") {
start();
}
virtual ~ThreadPoolSingleton() {
~ThreadPoolSingleton() {

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 15, 2017

Contributor

Nit: since the "override" keyword isn't being applied, I would prefer to keep the "virtual" keyword on virtual destructors to highlight that fact.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 15, 2017

Author Contributor

hi dillaman, thanks

FYI here isocpp/CppCoreGuidelines#737

CppCoreGuide said it's not prefer

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 15, 2017

Contributor

However, that violates the Ceph coding style (derived from Google's C++ Style Guide [1]):

Explicitly annotate overrides of virtual functions or virtual destructors with an override or (less frequently) final specifier. Older (pre-C++11) code will use the virtual keyword as an inferior alternative annotation. For clarity, use exactly one of override, final, or virtual when declaring an override. Rationale: A function or destructor marked override or final that is not an override of a base class virtual function will not compile, and this helps catch common errors. The specifiers serve as documentation; if no specifier is present, the reader has to check all ancestors of the class in question to determine if the function or destructor is virtual or not.

[1] https://google.github.io/styleguide/cppguide.html#Inheritance

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 16, 2017

Author Contributor

So, is ~destructor() override OK? I could make it automatically.

btw, @jcfischer @cbodley take a look please, we talked yet here [1]

[1] https://github.com/ceph/ceph/pull/13414/files/43c8b32ac7c57ae15b19e27bd69c04c75e296a4d#diff-89c2e65135de8a3db3f4ffe984ed8cc9

This comment has been minimized.

Copy link
@cbodley

cbodley Feb 16, 2017

Contributor

my comment there suggested removing an empty destructor to dodge this issue :) i agree with @dillaman that we should stick to our style guide here

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 16, 2017

Contributor

I'm fine w/ ~destructor() override

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 17, 2017

Author Contributor

Thanks @dillaman @cbodley , I will update this PR to ~destructor() override soon.

@liewegas need we unify other PR?

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 17, 2017

Author Contributor

I have updated.

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Feb 16, 2017

rbd: add override in rbd subsystem
Fixes: http://tracker.ceph.com/issues/18922

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:cleanup-rbd branch from bb97226 to 5fe3a10 Feb 17, 2017

@dillaman
Copy link
Contributor

dillaman left a comment

lgtm

@dillaman dillaman merged commit 50f7857 into ceph:master Feb 17, 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
@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 17, 2017

@Liuchang0812 just realized that this change didn't clean up everything in librbd -- see https://github.com/ceph/ceph/blob/master/src/librbd/AioImageRequest.h#L90 for an example

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 17, 2017

@dillaman Thanks for your tip. virtual func() override case couldn't be resolved. I will have a try to make it automatically.

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 17, 2017

@Liuchang0812 ... and there are a couple more examples below that one where "virtual" was left w/o the "override" keyword: https://github.com/ceph/ceph/blob/master/src/librbd/AioImageRequest.h#L93

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 17, 2017

@dillaman it seems that there are not introduced by this patch. Need I clean it?

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 17, 2017

Correct -- it wasn't introduced by this patch, but I would have thought the tool would have cleaned all the files(?).

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 17, 2017

No, clang-tidy only add override , it could not process this case. We could add this feature to it if we need this.

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 17, 2017

@Liuchang0812 what about the other cases where it was an override and it didn't add override and left virtual (line 93)?

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 17, 2017

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 17, 2017

@Liuchang0812 Quite possible -- I see lots of examples of missing "override" keywords in classes that derive from templated base-classes (which is a lot of classes in librbd).

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 17, 2017

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 17, 2017

@Liuchang0812 Thanks. BTW -- here is an example of a non-templated class where it failed to convert "virtual XYZ" to "XYZ override": https://github.com/ceph/ceph/blob/master/src/librbd/AioCompletion.h#L228

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 17, 2017

I think i already know why clang-tidy doesn't process *.h files. Tidy must know how to compile each files so that I generated a json file by cmake CMAKE_EXPORT_COMPILE_COMMANDS option which contains compile commands of *.cc files only.

I would try to fix it this weekend.

Thanks a lot @dillaman

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 20, 2017

@dillaman @liewegas I have fixed the problem that clang-type doesn't process header files. It will be a big changes. Should I make a PR?

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 20, 2017

@Liuchang0812 Sure -- I would do the same thing where you break it up by subsystem.

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 20, 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.