Fix move_if_noexcept usages in _Hashtable

[François Dumont]

I reviewed this patch following result of making std::pair piecewise 
constructor noexcept:

https://gcc.gnu.org/ml/libstdc++/2018-12/msg00052.html

I restore check of noexcept qualification on move constructor rather 
than alloc::construct cause when dealing with std::pair<const Key, 
Value> I want to check for noexcept qualification of Value type move 
constructor and it doesn't really make sens to check for 
alloc<Value>::construct when we eventually call alloc<pair<const Key, 
Value>::construct.

I'll submit this officially once back in stage 1.

François

On 12/4/18 10:43 PM, François Dumont wrote:
> On 12/4/18 3:38 PM, Jonathan Wakely wrote:
>> On 04/12/18 07:45 +0100, François Dumont wrote:
>>> Hi
>>>
>>> Â  This patch fix a minor problem with usage of 
>>> std::move_if_noexcept. We use it to move node content if move 
>>> construtor is noexcept but we eventually use the 
>>> allocator_type::construct method which might be slightly different. 
>>> I think it is better to check for this method noexcept qualification.
>>
>> This is likely to pessimize some code, since most allocators do not
>> have an exception-specification on their construct members.
>>
> Perhaps but the Standard mandates to call allocator construct so we 
> don't have choice. It is surprising to consider value_type move 
> constructor when we don't know what allocator construct does.
>
> Most users do not change from std::allocator or, even if they do, do 
> not implement construct so impact should be limited.
>
>>> Â  Moreover I have added a special overload for nodes containing a 
>>> std::pair. It is supposed to allow move semantic in associative 
>>> containers where Key is stored as const deleting std::pair move 
>>> constructor. In this case we should still move the Value part.
>>>
>>> Â  It doesn't work for the moment because the std::pair piecewise 
>>> constructor has no noexcept qualification. Is there any plan to add 
>>> it ? I think adding it will force including <tuple> in stl_pair.h, 
>>> is it fine ?
>
> No feedback on this point ? Is using std::pair piecewise constructor ok ?
>
>>>
>>> Â  If this evolution is accepted I'll adapt it for _Rb_tree that has 
>>> the same problem.
>>>
>>> Â  Working on this I also notice that content of initialization_list 
>>> is not moved. Is there a plan to make initialization_list iterator 
>>> type like move_iterator ? Should containers use 
>>> __make_move_iterator_if_noexcept ?
>>
>> No.
>>
>> Whether to allow moving from std::initializer_list is an active topic,
>> see
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1249r0.html
>>
> Ok, nice, allowing emplace build of values would be even better, I'll 
> have a closer look.
>
>>> Â  Tested under Linux x86_64 normal mode.
>>>
>>> Â  Ok to commit this first step ?
>>
>> No, this is not suitable for stage 3. It seems too risky.
>>
>> We can reconsider it during stage 1, but I'd like to see (at least) a
>> new test showing a bug with the current code. For example, a type with
>> a move constructor that is noexcept, but when used with a
>> scoped_allocator_adaptor (which calls something other than the move
>> constructor) we incorrectly move elements, and lose data when an
>> exception happens.
>>
>>
> Ok, I'll try.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hashtable.patch
Type: text/x-patch
Size: 16478 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20181216/1f45f5bb/attachment.bin>