Make basic_resolver_entry<> operators inline friend

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Make basic_resolver_entry<> operators inline friend

François Dumont-2
I am not yet sending this to gcc-patches because I am quite sceptical on
the approval.

While running tests with versioned namespace enabled those 2 tests are
failing:

experimental/net/internet/resolver/ops/lookup.cc
experimental/net/internet/resolver/ops/reverse.cc

This is because basic_resolver_entry<> == operator is using
basic_resolver_entry<>::host_name or service_name which expect a C++11
basic_string with the copy allocator aware constructor. But I haven't
enable C++11 abi string so the compilation error.

I don't know if the COW string implementation is supposed to have this
constructor but another way to fix those tests is to avoid those
members. By making operators friend we can access members directly and
so avoid the copy.

And yes, I also like inline friend operators :-)

What do you think ?

François



internet.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Make basic_resolver_entry<> operators inline friend

Jonathan Wakely-3
On 24/10/18 22:13 +0200, François Dumont wrote:

>I am not yet sending this to gcc-patches because I am quite sceptical
>on the approval.
>
>While running tests with versioned namespace enabled those 2 tests are
>failing:
>
>experimental/net/internet/resolver/ops/lookup.cc
>experimental/net/internet/resolver/ops/reverse.cc
>
>This is because basic_resolver_entry<> == operator is using
>basic_resolver_entry<>::host_name or service_name which expect a C++11
>basic_string with the copy allocator aware constructor. But I haven't
>enable C++11 abi string so the compilation error.
>
>I don't know if the COW string implementation is supposed to have this
>constructor

The COW string doesn't support C++11 allocators at all, and that
includes the allocator-extended constructors (although they'd be
pretty easy to add, without needing to support everything else for
C++11 allocators).

>but another way to fix those tests is to avoid those
>members. By making operators friend we can access members directly and
>so avoid the copy.
>
>And yes, I also like inline friend operators :-)
>
>What do you think ?

Not OK, the operators are specified in the TS.

(Maybe we should fix that, but until then it means they can't be
hidden friends).

It doesn't really fix the problem anyway. Those member functions still
doesn't work for the COW string, all this change does is hide the
problem by making a test pass.

We could just add { dg-require-effective-target cxx11-abi } to the
tests, but instead I think I'll add the allocator-extended
constructors to the COW string.


>François
>
>

>diff --git a/libstdc++-v3/include/experimental/internet b/libstdc++-v3/include/experimental/internet
>index 5e1dbb834db..e715fda163e 100644
>--- a/libstdc++-v3/include/experimental/internet
>+++ b/libstdc++-v3/include/experimental/internet
>@@ -1597,23 +1597,21 @@ namespace ip
>       basic_endpoint<_InternetProtocol> _M_ep;
>       string _M_host;
>       string _M_svc;
>-    };
>
>-  template<typename _InternetProtocol>
>-    inline bool
>-    operator==(const basic_resolver_entry<_InternetProtocol>& __a,
>-       const basic_resolver_entry<_InternetProtocol>& __b)
>-    {
>-      return __a.endpoint() == __b.endpoint()
>- && __a.host_name() == __b.host_name()
>- && __a.service_name() == __b.service_name();
>-    }
>+      friend bool
>+      operator==(const basic_resolver_entry& __a,
>+ const basic_resolver_entry& __b)
>+      {
>+ return __a._M_ep == __b._M_ep
>+  && __a._M_host == __b._M_host
>+  && __a._M_svc == __b._M_svc;
>+      }
>
>-  template<typename _InternetProtocol>
>-    inline bool
>-    operator!=(const basic_resolver_entry<_InternetProtocol>& __a,
>-       const basic_resolver_entry<_InternetProtocol>& __b)
>-    { return !(__a == __b); }
>+      friend bool
>+      operator!=(const basic_resolver_entry& __a,
>+ const basic_resolver_entry& __b)
>+      { return !(__a == __b); }
>+  };
>
>   // @}
>

Reply | Threaded
Open this post in threaded view
|

Re: Make basic_resolver_entry<> operators inline friend

François Dumont-2
On 10/25/18 1:23 PM, Jonathan Wakely wrote:

> On 24/10/18 22:13 +0200, François Dumont wrote:
>> I am not yet sending this to gcc-patches because I am quite sceptical
>> on the approval.
>>
>> While running tests with versioned namespace enabled those 2 tests
>> are failing:
>>
>> experimental/net/internet/resolver/ops/lookup.cc
>> experimental/net/internet/resolver/ops/reverse.cc
>>
>> This is because basic_resolver_entry<> == operator is using
>> basic_resolver_entry<>::host_name or service_name which expect a
>> C++11 basic_string with the copy allocator aware constructor. But I
>> haven't enable C++11 abi string so the compilation error.
>>
>> I don't know if the COW string implementation is supposed to have
>> this constructor
>
> The COW string doesn't support C++11 allocators at all, and that
> includes the allocator-extended constructors (although they'd be
> pretty easy to add, without needing to support everything else for
> C++11 allocators).
>
>> but another way to fix those tests is to avoid those members. By
>> making operators friend we can access members directly and so avoid
>> the copy.
>>
>> And yes, I also like inline friend operators :-)
>>
>> What do you think ?
>
> Not OK, the operators are specified in the TS.
>
> (Maybe we should fix that, but until then it means they can't be
> hidden friends).


Yes, I saw a lot of operators that would be better hidden in this file.
And in this case a friend operator avoids the


>
> It doesn't really fix the problem anyway. Those member functions still
> doesn't work for the COW string, all this change does is hide the
> problem by making a test pass.

Indeed.

>
> We could just add { dg-require-effective-target cxx11-abi } to the
> tests, but instead I think I'll add the allocator-extended
> constructors to the COW string.


Ok, I leave it to you then.

Thanks

François