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

include/mempool.h: fix Clangs complaint about types #13523

Merged
merged 1 commit into from Feb 28, 2017

Conversation

Projects
None yet
3 participants
@wjwithagen
Contributor

wjwithagen commented Feb 19, 2017

  • Clang generates the following error:

In file included from /home/jenkins/workspace/ceph-master/src/test/objectstore/store_test.cc:21:
In file included from /home/jenkins/workspace/ceph-master/src/os/ObjectStore.h:17:
In file included from /home/jenkins/workspace/ceph-master/src/include/Context.h:19:
In file included from /home/jenkins/workspace/ceph-master/src/common/dout.h:20:
In file included from /home/jenkins/workspace/ceph-master/src/common/config.h:21:
/usr/include/c++/v1/map:820:5: error: static_assert failed "Allocator::value_type must be same type as value_type"
static_assert((is_same<typename allocator_type::value_type, value_type>::value),
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jenkins/workspace/ceph-master/src/os/bluestore/bluestore_types.h:204:9: note: in instantiation of template class 'std::__1::map<unsigned long, bluestore_extent_ref_map_t::record_t, std::__1::less, mempool::pool_allocator<mempool::pool_index_t::mempool_bluestore_meta_other, std::__1::pair<unsigned long, bluestore_extent_ref_map_t::record_t> > >' requested here
map_t ref_map;

This is fixed by make the K in std::pair<k,v> a const.
Perhaps the other std::pair<> constuctors could do with a const
as well.

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

@wjwithagen wjwithagen requested review from tchaikov and liewegas Feb 19, 2017

@@ -376,7 +376,7 @@ class pool_allocator {
\
template<typename k,typename v, typename cmp = std::less<k> > \
using map = std::map<k, v, cmp, \
pool_allocator<std::pair<k,v>>>; \
pool_allocator<std::pair<const k,v>>>; \

This comment has been minimized.

@tchaikov

tchaikov Feb 19, 2017

Contributor

@wjwithagen could you add const to multimap and unordered_map's allocator's key type also? like:

diff --git a/src/include/mempool.h b/src/include/mempool.h
index 7b4a59618e..ad06fcd769 100644
--- a/src/include/mempool.h
+++ b/src/include/mempool.h
@@ -376,11 +376,11 @@ public:
                                                                         \
     template<typename k,typename v, typename cmp = std::less<k> >      \
     using map = std::map<k, v, cmp,                                    \
-                        pool_allocator<std::pair<k,v>>>;               \
+                        pool_allocator<std::pair<const k,v>>>;         \
                                                                         \
     template<typename k,typename v, typename cmp = std::less<k> >      \
     using multimap = std::multimap<k,v,cmp,                            \
-                                  pool_allocator<std::pair<k,v>>>;     \
+                                  pool_allocator<std::pair<const k,v>>>;       \
                                                                         \
     template<typename k, typename cmp = std::less<k> >                 \
     using set = std::set<k,cmp,pool_allocator<k>>;                     \
@@ -395,7 +395,7 @@ public:
             typename h=std::hash<k>,                                   \
             typename eq = std::equal_to<k>>                            \
     using unordered_map =                                              \
-      std::unordered_map<k,v,h,eq,pool_allocator<std::pair<k,v>>>;     \
+      std::unordered_map<k,v,h,eq,pool_allocator<std::pair<const k,v>>>;       \
                                                                         \
     inline size_t allocated_bytes() {                                  \
       return mempool::get_pool(id).allocated_bytes();                  \

because we should use pair<const Key, T> as the value type of allocator as per § 23.4.4.1, n3337.

namespace std {
  template <class Key, class T, class Compare = less<Key>,
            class Allocator = allocator<pair<const Key, T> > >
  class map {
  public:
    // types:
    typedef Key                  key_type;
    typedef T                    mapped_type;
    typedef pair<const Key, T>   value_type;
    // ...
  };

the same applies to other associative containers containing key-value pairs. both GCC and Clang comply to this standard. but libstdc++ does not do the static_assert(), so we are not aware of this problem until now.

This comment has been minimized.

@wjwithagen

wjwithagen Feb 19, 2017

Contributor

@tchaikov
I held back on that because I wasn't sure the issue is there as well.
But I'll put them in as well with these routines.

@tchaikov tchaikov added the needs-qa label Feb 19, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Feb 19, 2017

@wjwithagen could you update the commit message

  1. remove "Perhaps the other std::pair<> constuctors could do with a const as well"
  2. might want to reference the related C++11 standard section also, where the typedef pair<const Key, T> value_type is stated as a part of the standard.

thanks!

include/mempool.h: fix Clangs complaint about types
 - Clang generates the following error:

In file included from /home/jenkins/workspace/ceph-master/src/test/objectstore/store_test.cc:21:
In file included from /home/jenkins/workspace/ceph-master/src/os/ObjectStore.h:17:
In file included from /home/jenkins/workspace/ceph-master/src/include/Context.h:19:
In file included from /home/jenkins/workspace/ceph-master/src/common/dout.h:20:
In file included from /home/jenkins/workspace/ceph-master/src/common/config.h:21:
/usr/include/c++/v1/map:820:5: error: static_assert failed "Allocator::value_type must be same type as value_type"
    static_assert((is_same<typename allocator_type::value_type, value_type>::value),
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jenkins/workspace/ceph-master/src/os/bluestore/bluestore_types.h:204:9: note: in instantiation of template class 'std::__1::map<unsigned long, bluestore_extent_ref_map_t::record_t, std::__1::less<unsigned long>, mempool::pool_allocator<mempool::pool_index_t::mempool_bluestore_meta_other, std::__1::pair<unsigned long, bluestore_extent_ref_map_t::record_t> > >' requested here
  map_t ref_map;

This is fixed by make the K in std::pair<k,v> a const.

 - Based on the C++ we should use pair<const Key, T> as the value
   type of allocator as per C++ standard par. 23.4.4.1, n3337.

namespace std {
  template <class Key, class T, class Compare = less<Key>,
            class Allocator = allocator<pair<const Key, T> > >
  class map {
  public:
    // types:
    typedef Key                  key_type;
    typedef T                    mapped_type;
    typedef pair<const Key, T>   value_type;
    // ...
  };

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

@tchaikov tchaikov merged commit 1f0824b into ceph:master Feb 28, 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 wjwithagen deleted the wjwithagen:wip-wjw-clang-mempool branch Feb 28, 2017

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