Re: [Patch] Fix variant::operator= on references

[Jonathan Wakely]

On 22/09/16 03:40 -0700, Tim Shen wrote:

On Thu, Sep 22, 2016 at 3:36 AM, Tim Shen  wrote:

Then my question is, what about type traits uses like
is_copy_constructible? I have seen non-qualified uses in std::any and
std::optional and other places. Should all of them be qualified?


Ah never mind, I realized that *usually* a type trait use is not part
of a function call, so ADL is not triggered.


ADL is only used to do name lookup for unqualified functions, so it is
never necessary to qualify those types inside namespace std. Name
lookup will always find the right type.

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 3:36 AM, Tim Shen  wrote:
> Then my question is, what about type traits uses like
> is_copy_constructible? I have seen non-qualified uses in std::any and
> std::optional and other places. Should all of them be qualified?

Ah never mind, I realized that *usually* a type trait use is not part
of a function call, so ADL is not triggered.

-- 
Regards,
Tim Shen

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 3:03 AM, Jonathan Wakely  wrote:
> On 22/09/16 01:49 -0700, Tim Shen wrote:
>>
>> Done. When writing the initial version, I was trying to save as much
>> qualifications as possible (as long as the semantic doesn't change)
>> for readability, but that might not be a good idea.
>
>
> It does change the semantics, as forward<_Tp>(__tp) can find another
> function via ADL (see the new test in this patch).

Then my question is, what about type traits uses like
is_copy_constructible? I have seen non-qualified uses in std::any and
std::optional and other places. Should all of them be qualified?

>
> Tested powerpc64le-linux, committed to trunk.
>
>



-- 
Regards,
Tim Shen

Re: [Patch] Fix variant::operator= on references

[Ville Voutilainen]

On 22 September 2016 at 13:03, Jonathan Wakely  wrote:
> On 22/09/16 01:49 -0700, Tim Shen wrote:
>>
>> Done. When writing the initial version, I was trying to save as much
>> qualifications as possible (as long as the semantic doesn't change)
>> for readability, but that might not be a good idea.
>
>
> It does change the semantics, as forward<_Tp>(__tp) can find another
> function via ADL (see the new test in this patch).


Yeah, it's not a question about readability or style, unqualified
function calls attract
the ADL demons like fairies attract vampires.

Re: [Patch] Fix variant::operator= on references

[Jonathan Wakely]

On 22/09/16 01:49 -0700, Tim Shen wrote:

Done. When writing the initial version, I was trying to save as much
qualifications as possible (as long as the semantic doesn't change)
for readability, but that might not be a good idea.


It does change the semantics, as forward<_Tp>(__tp) can find another
function via ADL (see the new test in this patch).

Tested powerpc64le-linux, committed to trunk.


commit fe8a92068c783bd2a911c1864c62ffd8c3f31ea1
Author: Jonathan Wakely 
Date:   Thu Sep 22 10:45:50 2016 +0100

Always qualify std::forward in 

	* include/bits/uses_allocator.h (__uses_allocator_construct): Qualify
	std::forward and ::new. Cast pointer to void*.
	* include/std/variant (_Variant_storage, _Union, _Variant_base)
	(__access, __visit_invoke, variant, visit): Qualify std::forward.
	* testsuite/20_util/variant/compile.cc: Test for ADL problems.

diff --git a/libstdc++-v3/include/bits/uses_allocator.h b/libstdc++-v3/include/bits/uses_allocator.h
index 500bd90..c7d14f3 100644
--- a/libstdc++-v3/include/bits/uses_allocator.h
+++ b/libstdc++-v3/include/bits/uses_allocator.h
@@ -144,24 +144,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template
 void __uses_allocator_construct_impl(__uses_alloc0 __a, _Tp* __ptr,
 	 _Args&&... __args)
-{ new (__ptr) _Tp(forward<_Args>(__args)...); }
+{ ::new ((void*)__ptr) _Tp(std::forward<_Args>(__args)...); }
 
   template
 void __uses_allocator_construct_impl(__uses_alloc1<_Alloc> __a, _Tp* __ptr,
 	 _Args&&... __args)
-{ new (__ptr) _Tp(allocator_arg, *__a._M_a, forward<_Args>(__args)...); }
+{
+  ::new ((void*)__ptr) _Tp(allocator_arg, *__a._M_a,
+			   std::forward<_Args>(__args)...);
+}
 
   template
 void __uses_allocator_construct_impl(__uses_alloc2<_Alloc> __a, _Tp* __ptr,
 	 _Args&&... __args)
-{ new (__ptr) _Tp(forward<_Args>(__args)..., *__a._M_a); }
+{ ::new ((void*)__ptr) _Tp(std::forward<_Args>(__args)..., *__a._M_a); }
 
   template
 void __uses_allocator_construct(const _Alloc& __a, _Tp* __ptr,
 _Args&&... __args)
 {
   __uses_allocator_construct_impl(__use_alloc<_Tp, _Alloc, _Args...>(__a),
-  __ptr, forward<_Args>(__args)...);
+  __ptr, std::forward<_Args>(__args)...);
 }
 
 _GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 1ad33fc..ac483f3 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -298,7 +298,7 @@ namespace __variant
 
   template
 	constexpr _Variant_storage(in_place_index_t<_Np>, _Args&&... __args)
-	: _M_union(in_place<_Np>, forward<_Args>(__args)...)
+	: _M_union(in_place<_Np>, std::forward<_Args>(__args)...)
 	{ }
 
   ~_Variant_storage() = default;
@@ -316,13 +316,13 @@ namespace __variant
 
 	template
 	  constexpr _Union(in_place_index_t<0>, _Args&&... __args)
-	  : _M_first(in_place<0>, forward<_Args>(__args)...)
+	  : _M_first(in_place<0>, std::forward<_Args>(__args)...)
 	  { }
 
 	template>
 	  constexpr _Union(in_place_index_t<_Np>, _Args&&... __args)
-	  : _M_rest(in_place<_Np - 1>, forward<_Args>(__args)...)
+	  : _M_rest(in_place<_Np - 1>, std::forward<_Args>(__args)...)
 	  { }
 
 	_Uninitialized<__storage<_First>> _M_first;
@@ -386,7 +386,7 @@ namespace __variant
   template
 	constexpr explicit
 	_Variant_base(in_place_index_t<_Np> __i, _Args&&... __args)
-	: _Storage(__i, forward<_Args>(__args)...), _M_index(_Np)
+	: _Storage(__i, std::forward<_Args>(__args)...), _M_index(_Np)
 	{ }
 
   template
@@ -426,7 +426,7 @@ namespace __variant
 	  using _Storage =
 	__storage>>;
 	  __uses_allocator_construct(__a, static_cast<_Storage*>(_M_storage()),
- forward<_Args>(__args)...);
+ std::forward<_Args>(__args)...);
 	  __glibcxx_assert(_M_index == _Np);
 	}
 
@@ -581,7 +581,7 @@ namespace __variant
 decltype(auto) __access(_Variant&& __v)
 {
   return __get_alternative<__reserved_type_map<_Variant&&, __storage<_Tp>>>(
-	__get_storage(forward<_Variant>(__v)));
+	__get_storage(std::forward<_Variant>(__v)));
 }
 
   // A helper used to create variadic number of _To types.
@@ -591,10 +591,11 @@ namespace __variant
   // Call the actual visitor.
   // _Args are qualified storage types.
   template
-decltype(auto) __visit_invoke(_Visitor&& __visitor,
-  _To_type<_Args, void*>... __ptrs)
+decltype(auto)
+__visit_invoke(_Visitor&& __visitor, _To_type<_Args, void*>... __ptrs)
 {
-  return forward<_Visitor>(__visitor)(__get_alternative<_Args>(__ptrs)...);
+  return std::forward<_Visitor>(__visitor)(
+	  __get_alternative<_Args>(__ptrs)...);
 }
 
   // Used for storing multi-dimensional vtable.
@@ -1010,7 +1011,7 @@ namespace __variant
 	constexpr
 	variant(_Tp&& __t)
 	noexcept(is_nothrow_constructible_v<__accepted_type<_Tp&&>, _Tp&&>)
-	: 

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 1:39 AM, Jonathan Wakely  wrote:
> Please qualify std::forward here.

Done. When writing the initial version, I was trying to save as much
qualifications as possible (as long as the semantic doesn't change)
for readability, but that might not be a good idea.

>
> OK for trunk with that change, thanks for the quick fix.
>
>

Committed. Thanks!


-- 
Regards,
Tim Shen

Re: [Patch] Fix variant::operator= on references

[Jonathan Wakely]

On 22/09/16 00:43 -0700, Tim Shen wrote:

Hi, this patch fixes the following compilation failure:

#include 

int main()
{
  float f1 = 1.0f, f2 = 2.0f;

  std::variant v1(f1);

  v1 = f2; // #1
}

The bug is caused by a misuse of __storage. I also examined other
__storage usage, they all seem appropriate.


@@ -1147,8 +1147,7 @@ namespace __variant
   {
 constexpr auto __index = __accepted_index<_Tp&&>;
 if (index() == __index)
-   *static_cast<__storage<__to_type<__index>>*>(this->_M_storage())
- = forward<_Tp>(__rhs);
+   std::get<__index>(*this) = forward<_Tp>(__rhs);

Please qualify std::forward here.

OK for trunk with that change, thanks for the quick fix.

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 12:43 AM, Tim Shen  wrote:
> Hi, this patch fixes the following compilation failure:

For the record, the bug is found by Ville. Thank you Ville! :)


-- 
Regards,
Tim Shen