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

common: Remove redundant includes #15003

Merged
merged 1 commit into from May 10, 2017

Conversation

Projects
None yet
4 participants
@badone
Contributor

badone commented May 9, 2017

This is the first of many PRs for this. Reducing the amount of redundant includes should help to improve compilation times and reduce the potential for confusion. The result should be a cleaner codebase.

Fixes: http://tracker.ceph.com/issues/19883 (Partially)

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

common: Remove redundant includes
Fixes: http://tracker.ceph.com/issues/19883 (Partially)

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

lgtm!

@liewegas liewegas added needs-qa and removed needs-review labels May 9, 2017

@joscollin

This comment has been minimized.

Member

joscollin commented May 9, 2017

@badone This is a good idea and there are a lot of files that needs this kind of cleanup. But as per my discussion with @liewegas and @tchaikov earlier, cleanups like this obscure the history. So considering the necessity of doing this cleanup, I would suggest to have an idea like a separate branch or something that doesn't affect the history.

I have started doing the same cleanup earlier and stopped it after cleaning one file because of this reason.

@liewegas

This comment has been minimized.

Member

liewegas commented May 9, 2017

@joscollin There isn't a problem here with obscuring the history... It's automated updates that fix things like whitespace or otherwise touch a large number of lines of code that make git blame hard to use.

@liewegas

This comment has been minimized.

Member

liewegas commented May 9, 2017

There's a concrete benefit to pruning the #includes in reduce compile times. I can't remember what the earlier change was that you were working on.. do you have a PR link?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 9, 2017

IIRC, @cxwshawn posted a handful of PR working in this direction: https://github.com/ceph/ceph/pulls?utf8=✓&q=is%3Apr%20author%3Acxwshawn%20dependency , quite a few of them got merged. but the others were closed by him before being merged.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 9, 2017

the #includes convey much less information by themselves than the other part of source code, and as @liewegas said, it reduces the compilation time and even helps with the readability sometimes. so i guess it does not obscure the history in the degree of modernizing the code using C++11 syntax.

@joscollin

This comment has been minimized.

Member

joscollin commented May 9, 2017

I can't remember what the earlier change was that you were working on.. do you have a PR link?

@liewegas @tchaikov I was referring to the following PRs, in which we had a discussion about this matter.
#14381
#14183

This is the first of many PRs for this.

@badone Basically, I'm in favour of these kind of cleanups. But your (Sage and Kefu) suggestions are also good. So I wonder if there is another way of executing these kind of cleanup work, which doesn't obscure the git history. I was thinking: Is there an option in git or any other way of executing these cleanup tasks ?

@badone

This comment has been minimized.

Contributor

badone commented May 10, 2017

@joscollin no way I know of but I'm open to suggestions of course. If you are advocating we leave redundant includes to avoid disturbing git history I suggest you think again. These are not just cosmetic changes, they have performance impact.

@liewegas liewegas merged commit 696f061 into ceph:master May 10, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment