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 - 3 #15204

Merged
merged 1 commit into from May 23, 2017

Conversation

Projects
None yet
5 participants
@joscollin
Member

joscollin commented May 22, 2017

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

Signed-off-by: Jos Collin jcollin@redhat.com

@joscollin joscollin requested a review from badone May 22, 2017

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

Signed-off-by: Jos Collin <jcollin@redhat.com>

@liewegas liewegas merged commit 65b144d into ceph:master May 23, 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
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented on src/common/io_priority.cc in edbd6ed May 26, 2017

@joscollin
@liewegas
@tchaikov
Hi Guys,
There where a lot of includefile reorganises done.

But now I have to fix this again back for some, because under FreeBSD/Clang these includes need to be the way they are...

First: On of the rules in the code-guides Ceph suggests to use is that the order of includes is:

  • Standard inlcudes
  • local includes.

As a result of this reversal pid_t is not available in CLANG because it comes from <unistd.h>

And error nummers in FreeBSD specifically live in errno.h

So I'm having a rather hard time figuring out if compiles are now breaking due to these "fixes"of due to other things where Clang is just genearly unhappy.

Sorry for the rant, but I'm rather sad at the moment.

This comment has been minimized.

Contributor

tchaikov replied May 26, 2017

@wjwithagen i am really sorry for breaking the build on FreeBSD. and practically reverting your work. i will loop you in next time whenever possible. but in the meanwhile, maybe we (you) could bisect and fin out the commit that fails the build so we can fix it?

@badone ^

This comment has been minimized.

Contributor

wjwithagen replied May 26, 2017

@tchaikov @badone

No worries, I'm a bit frustrated, but then we fix it and get done with it.
I'm almost back at 78% thru the compilation. So give or take some more glitches, it'll get done. :)

This comment has been minimized.

Member

joscollin replied May 26, 2017

This comment has been minimized.

Contributor

tchaikov replied May 26, 2017

@wjwithagen i can help to get it build with clang on GNU/Linux this week-end if this can lower your frustration level.

This comment has been minimized.

Contributor

wjwithagen replied May 26, 2017

@tchaikov
Thanx for the moral support.

Frustration is/was rather limited, and already gone...
It is more like: I'm off for 2 weeks, and right in that slot all this got committed. Damn...

Changes are not that major, but it is more figuring out what are the essentials.
I was able to completely build my WIP, so now I'll start running tests again.
So over the weekend you'll see a PR with some workables for FreeBSD.

This comment has been minimized.

Contributor

tchaikov replied May 26, 2017

okay!

@joscollin

This comment has been minimized.

Member

joscollin commented May 26, 2017

@wjwithagen : Sorry to hear that it breaks in FreeBSD. I think the best way to optimize the includes is to proceed fixing them in FreeBSD too.

@liewegas @tchaikov : While doing these series of PRs, which was initiated by @badone, I was careful not to remove the includes under #ifdef s for different architectures and other suspicious ones, so that I can avoid problem that may occur in other platforms. I have tested these fixes in my system, which is Fedora/Rawhide x86_64. And I was expecting fixes when people test in other platforms too.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented on src/common/ipaddr.cc in edbd6ed May 26, 2017

@joscollin
@liewegas

Same here....

@joscollin

This comment has been minimized.

Member

joscollin commented May 26, 2017

@liewegas @wjwithagen

Standard inlcudes
local includes.
Same here....

The order of the includes was not strictly followed in many files. I have found instances where it starts with Standard Includes, then comes local includes and then Standard Includes again. Sometimes it is mixed too. So it was confusing. However I have put an effort to group them together.

@wjwithagen By the way, please note that the order was not changed in ipaddr.cc

@badone

This comment has been minimized.

Contributor

badone commented May 26, 2017

@wjwithagen The question I would have is, out of the hundreds of header changes in these PRs specifically how many affected your FreeBSD build and can you tell us which ones?

@joscollin joscollin deleted the joscollin:wip-cleanup-redundant-headers-3 branch Jun 8, 2017

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