nlohmann / json

JSON for Modern C++
https://json.nlohmann.me
MIT License
42.18k stars 6.65k forks source link

custom allocators: define missing 'rebind' type #3895

Closed trofi closed 1 year ago

trofi commented 1 year ago

gcc-13 added an assert to standard headers to make sure custom allocators have intended implementation of rebind type instead of inherited rebind. gcc change: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=64c986b49558a7

Without the fix build fails on this week's gcc-13 as:

In file included from <<NIX>>-gcc-13.0.0/include/c++/13.0.0/ext/alloc_traits.h:34,
                 from <<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/basic_string.h:39,
                 from <<NIX>>-gcc-13.0.0/include/c++/13.0.0/string:54,
                 from <<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/locale_classes.h:40,
                 from <<NIX>>-gcc-13.0.0/include/c++/13.0.0/locale:41,
                 from tests/src/unit-regression2.cpp:19:
<<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h: In instantiation of 'struct std::__allocator_traits_base::__rebind<my_allocator<unsigned char>, unsigned char, void>':
<<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:94:11:   required by substitution of 'template<class _Alloc, class _Up> using std::__alloc_rebind = typename std::__allocator_traits_base::__rebind<_Alloc, _Up>::type [with _Alloc = my_allocator<unsigned char>; _Up = unsigned char]'
<<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:228:8:   required by substitution of 'template<class _Alloc> template<class _Tp> using std::allocator_traits< <template-parameter-1-1> >::rebind_alloc = std::__alloc_rebind<_Alloc, _Tp> [with _Tp = unsigned char; _Alloc = my_allocator<unsigned char>]'
<<NIX>>-gcc-13.0.0/include/c++/13.0.0/ext/alloc_traits.h:126:65:   required from 'struct __gnu_cxx::__alloc_traits<my_allocator<unsigned char>, unsigned char>::rebind<unsigned char>'
<<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/stl_vector.h:88:21:   required from 'struct std::_Vector_base<unsigned char, my_allocator<unsigned char> >'
<<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/stl_vector.h:423:11:   required from 'class std::vector<unsigned char, my_allocator<unsigned char> >'
tests/src/unit-regression2.cpp:807:63:   required from here
<<NIX>>-gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:70:31: error: static assertion failed: allocator_traits<A>::rebind_alloc<A::value_type> must be A
   70 |                         _Tp>::value,
      |                               ^~~~~

The change adds trivial rebind definition with expected return type.

[Describe your pull request here. Please read the text below the line, and make sure you follow the checklist.]


Pull request checklist

Read the Contribution Guidelines for detailed information.

Please don't

coveralls commented 1 year ago

Coverage Status

Coverage remained the same at 100.0% when pulling a5b09d50b786638ed9deb09ef13860a3cb64eb6b on trofi:gcc-13-rebind-fix into a2f0593649e23c51f9b5aa7480f125d2bd8bd83b on nlohmann:develop.

navinp0304 commented 1 year ago

your changes in a5b09d5 is correct. Somehow the rebind part didn't get merged. I created PR #3966 for the missed change.

trofi commented 1 year ago

I'm confused. Can you clarify why you need your PR? It looks incomplete compared to this one and will create a conflict if merged.

navinp0304 commented 1 year ago

I'm confused. Can you clarify why you need your PR? It looks incomplete compared to this one and will create a conflict if merged.

You haven't pushed the struct rebind other part. I can close my PR if you merge the struct rebind.

trofi commented 1 year ago

I'm confused. Can you clarify why you need your PR? It looks incomplete compared to this one and will create a conflict if merged.

You haven't pushed the struct rebind other part. I can close my PR if you merge the struct rebind.

Can you clarify where you expect it to be? https://github.com/nlohmann/json/commit/a5b09d50b786638ed9deb09ef13860a3cb64eb6b.patch does show rebind::other presence for both classes:

diff --git a/tests/src/unit-allocator.cpp b/tests/src/unit-allocator.cpp
index c6b77ed669..c3708e89e3 100644
--- a/tests/src/unit-allocator.cpp
+++ b/tests/src/unit-allocator.cpp
@@ -20,11 +20,20 @@ struct bad_allocator : std::allocator<T>
 {
     using std::allocator<T>::allocator;

+    bad_allocator() = default;
+    template<class U> bad_allocator(const bad_allocator<U>& /*unused*/) { }
+
     template<class... Args>
     void construct(T* /*unused*/, Args&& ... /*unused*/)
     {
         throw std::bad_alloc();
     }
+
+    template <class U>
+    struct rebind
+    {
+        using other = bad_allocator<U>;
+    };
 };
 } // namespace

--- a/tests/src/unit-regression2.cpp
+++ b/tests/src/unit-regression2.cpp
@@ -187,6 +187,15 @@ class my_allocator : public std::allocator<T>
 {
   public:
     using std::allocator<T>::allocator;
+
+    my_allocator() = default;
+    template<class U> my_allocator(const my_allocator<U>& /*unused*/) { }
+
+    template <class U>
+    struct rebind
+    {
+        using other = my_allocator<U>;
+    };
 };

 /////////////////////////////////////////////////////////////////////

Do you expect more code here? Or somewhere else?

navinp0304 commented 1 year ago

I'm confused. Can you clarify why you need your PR? It looks incomplete compared to this one and will create a conflict if merged.

You haven't pushed the struct rebind other part. I can close my PR if you merge the struct rebind.

Can you clarify where you expect it to be? https://github.com/nlohmann/json/commit/a5b09d50b786638ed9deb09ef13860a3cb64eb6b.patch does show rebind::other presence for both classes:

Do you expect more code here? Or somewhere else?

See this link https://github.com/nlohmann/json/compare/274f83e115b0e9af8f98f940b927d83455ba419b..a5b09d50b786638ed9deb09ef13860a3cb64eb6b

trofi commented 1 year ago

I still don't follow. Sorry.

navinp0304 commented 1 year ago

I still don't follow. Sorry.

Are your changes merged to develop branch ?

trofi commented 1 year ago

No. This is an open PR against develop branch.

navinp0304 commented 1 year ago

No. This is an open PR against develop branch.

Ok, i closed mine. I can confirm your changes work.

nlohmann commented 1 year ago

Thanks a lot!