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 #15019

Merged
merged 1 commit into from May 12, 2017

Conversation

Projects
None yet
2 participants
@badone
Contributor

badone commented May 10, 2017

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

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

#include <string>
#include <map>
#include "include/buffer_fwd.h"

This comment has been minimized.

@tchaikov

tchaikov May 10, 2017

Contributor

i think buffer_fwd.h is a better choice? as it just forward-declares the buffer types instead of including the full definitions of them, which slow down the compilation.

This comment has been minimized.

@badone

badone May 10, 2017

Contributor

You are looking at this in isolation. Formatter.cc needs buffer.h

This comment has been minimized.

@tchaikov

tchaikov May 10, 2017

Contributor

@badone Formatter.cc should include "buffer.h" as it is doing now.

@@ -11,7 +13,7 @@ void dump(const SloppyCRCMap& scm)
f->open_object_section("map");
scm.dump(f);
f->close_section();
f->flush(cout);
f->flush(std::cout);

This comment has been minimized.

@tchaikov

tchaikov May 10, 2017

Contributor

i think, next time, we can just put a using namespace std in the front of .cc file to avoid the code churn?

This comment has been minimized.

@badone

badone May 10, 2017

Contributor

That doesn't bother you in terms of potential namespace pollution?

This comment has been minimized.

@tchaikov

tchaikov May 10, 2017

Contributor

@badone i am less concerned about this problem in .cc files.

@@ -14,10 +14,11 @@
#include "acconfig.h"
#include "common/pipe.h"
#include "include/compat.h"

This comment has been minimized.

@tchaikov

tchaikov May 10, 2017

Contributor

i am not sure why we want to remove this. VOID_TEMP_FAILURE_RETRY() is defined in compat.h.

@@ -159,7 +158,7 @@ void SloppyCRCMap::dump(Formatter *f) const
{
f->dump_unsigned("block_size", block_size);
f->open_array_section("crc_map");
for (map<uint64_t,uint32_t>::const_iterator p = crc_map.begin(); p != crc_map.end(); ++p) {
for (std::map<uint64_t,uint32_t>::const_iterator p = crc_map.begin(); p != crc_map.end(); ++p) {

This comment has been minimized.

@tchaikov

tchaikov May 10, 2017

Contributor

if we really need to change this. could use auto also. but i'd recommend using namespace std to avoid the code churn.

This comment has been minimized.

@badone

badone May 10, 2017

Contributor

That doesn't bother you in terms of potential namespace pollution?

This comment has been minimized.

@tchaikov

tchaikov May 10, 2017

Contributor

ditto, .cc is safer when it comes to namespace pollution.

@badone

This comment has been minimized.

Contributor

badone commented May 10, 2017

@tchaikov ack, fixing...

@badone

This comment has been minimized.

Contributor

badone commented May 10, 2017

@tchaikov in some of these files I have replaced "include/int_types.h" with <stdint.h>. I'm thinking that might cause problems?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 10, 2017

@badone i am not sure. on some non-linux platforms __u8 and its friends are not defined. if we replace "include/int_types.h" with <stdint.h>, and we are using __u8 types, there are chances we will have FTBFS on those platforms.

but we can surely ditch the definitions of PRIu64s.

@badone

This comment has been minimized.

Contributor

badone commented May 10, 2017

@tchaikov Not sure what I can do about the PRIu64 defs but let me back out the stdint.h changes to be safe.

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

Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
@@ -19,6 +19,7 @@
#include <sstream>
#include <string>
using namespace ceph;

This comment has been minimized.

@tchaikov

tchaikov May 11, 2017

Contributor

nit could remove the std::ostringstream; below this line.

This comment has been minimized.

@badone

badone May 11, 2017

Contributor

Needed unless I fully qualify all ostringstream references

@tchaikov

lgtm

@tchaikov tchaikov merged commit 5a32cd3 into ceph:master May 12, 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