Re: Fix move_if_noexcept usages in _Hashtable

["François Dumont" <frs.dumont@gmail.com>]

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.