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

rgw/rgw_string.h: FreeBSD would like errno.h included #15737

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
3 participants
@wjwithagen
Contributor

wjwithagen commented Jun 16, 2017

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@@ -12,6 +12,7 @@
*
*/
#include <errno.h>

This comment has been minimized.

@cbodley

cbodley Jun 16, 2017

Contributor

thanks @wjwithagen. i had that #include "rgw/rgw_string.h" first to make sure the header could be included on its own. it sounds like it's the header that has a dependency on <errno.h>, so i'd prefer to move the include there

This comment has been minimized.

@wjwithagen

wjwithagen Jun 16, 2017

Contributor

@cbodley
I'm not sure what the project stanza is on this type of includes in include files.
Both types are present.

This comment has been minimized.

@cbodley

cbodley Jun 16, 2017

Contributor

sorry, i don't follow..

if you'd have to include errno.h in every file that included rgw_string.h, surely rgw_string.h should just include errno.h instead

or am i missing the actual error here?

This comment has been minimized.

@wjwithagen

wjwithagen Jun 16, 2017

Contributor

@cbodley
What I meant is that both options are used in the code.
Include files that require other include files to be included first.
And include files that include those files themselves.

I guess it is up to the author how he wants to solve it.
Personally I prefer include files to include their own requirements.

This comment has been minimized.

@cbodley

cbodley Jun 16, 2017

Contributor

okay, yeah

Include files that require other include files to be included first.

i would guess that most of those cases arise by accident, rather than by design. the google c++ style guide is pretty explicit about this, putting the section about Self-contained headers at the very top

This comment has been minimized.

@cbodley

cbodley Jun 19, 2017

Contributor

can you please move the include to rgw_string.h? i'm happy to merge once that builds

This comment has been minimized.

@wjwithagen

wjwithagen Jun 19, 2017

Contributor

@cbodley
Fixed the PR, now see if it builds

@wjwithagen wjwithagen changed the title from test_rgw_string.cc: FreeBSD would like errno.h included to rgw_string.h: FreeBSD would like errno.h included Jun 19, 2017

rgw_string.h: FreeBSD would like errno.h included
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

@liewegas liewegas changed the title from rgw_string.h: FreeBSD would like errno.h included to rgw/rgw_string.h: FreeBSD would like errno.h included Jun 20, 2017

@liewegas liewegas added the needs-qa label Jun 20, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 20, 2017

@liewegas this fixes compilation of a unit test, so needs-qa is probably overkill

p.s. does that make check failure in test_objectstore_memstore.sh look familiar?

@liewegas liewegas removed the needs-qa label Jun 20, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 20, 2017

There are occasional memstore crashes, yeah. Haven't looked into it.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 20, 2017

jenkins test this please

@cbodley cbodley merged commit 1b33440 into ceph:master Jun 20, 2017

3 of 4 checks passed

arm64 make check arm64 make check failed
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment