[libcxx-commits] [PATCH] D58587: Implement P0433: deduction guides for <map>

[Louis Dionne via libcxx-commits]

> On Jun 17, 2019, at 14:50, Michał Górny <mgorny at gentoo.org> wrote:
> 
> On Mon, 2019-06-17 at 13:49 -0400, Arthur O'Dwyer wrote:
>> On Mon, Jun 17, 2019 at 12:30 PM Michał Górny via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>> 
>>> mgorny added a comment.
>>> 
>>> Marshall, `std/containers/associative/map/map.cons/deduct_const.pass.cpp`
>>> test seems to be reliably failing from commit one on both Gentoo Linux and
>>> NetBSD. Could you please look at the classical example of totally cryptic
>>> C++ error? ;-)
>>> 
>>> 
>>> http://lab.llvm.org:8011/builders/netbsd-amd64/builds/20/steps/run%20unit%20tests/logs/FAIL%3A%20libc%2B%2B%3A%3Adeduct_const.pass.cpp
>>> 
>> 
>> Well, I can contribute that that's the error that you get when CTAD deduces
>> the wrong thing. Here it's looking at lines 98–100 of deduct_const.pass.cpp
>> 
>>    std::map m({ PC{1,1L}, PC{2,2L}, PC{1,1L}, PC{INT_MAX,1L}, PC{3,1L} },
>> test_allocator<PC>(0, 45));
>>    ASSERT_SAME_TYPE(decltype(m), std::map<int, long, std::less<int>,
>> test_allocator<PC>>);
>> 
>> and deducing the correct value_type for `m`, but it's incorrectly thinking
>> that `test_allocator<PC>` should be used as the *comparator* type, not
>> the *allocator
>> *type.  This is surprising/incorrect, because if all is going as expected,
>> then CTAD should be considering the deduction guides
>> 
>> template<class _Key, class _Tp, class _Compare = less<typename
>> remove_const<_Key>::type>,
>>         class _Allocator = allocator<pair<const _Key, _Tp>>,
>>         class = typename enable_if<!__is_allocator<_Compare>::value,
>> void>::type,
>>         class = typename enable_if<__is_allocator<_Allocator>::value,
>> void>::type>
>> map(initializer_list<pair<_Key, _Tp>>, _Compare = _Compare(), _Allocator =
>> _Allocator())
>>  -> map<typename remove_const<_Key>::type, _Tp, _Compare, _Allocator>;
>> 
>> template<class _Key, class _Tp, class _Allocator,
>>         class = typename enable_if<__is_allocator<_Allocator>::value,
>> void>::type>
>> map(initializer_list<pair<_Key, _Tp>>, _Allocator)
>>  -> map<typename remove_const<_Key>::type, _Tp, less<typename
>> remove_const<_Key>::type>, _Allocator>;
>> 
>> CTAD should see that the first guide fails substitution because
>> __is_allocator<test_allocator<PC>>::value (that is, test_allocator should
>> not be taken as the comparator type).
>> CTAD should see that the second guide does not fail substitution, so CTAD
>> should unambiguously use the second guide.
>> 
>> The test passes on OSX using clang trunk.  I don't see any #ifdefs or
>> anything in include/ or in test/support/test_allocator.h that would cause
>> different behavior on Linux or NetBSD.  If you try clang trunk on Linux or
>> NetBSD, do you see different behavior (versus what you see with clang
>> 9.0.0)?
> 
> This is trunk.  On NetBSD, it's tested using a single tree with
> cxx_under_test being pointed to just-built clang.  On Gentoo, I was
> testing out-of-source build against ~same revision of trunk (± delay
> between fetching the two repos separately).
> 
> And yes, I'm surprised that Debian buildbots don't hit this.

So you're hitting that with Clang trunk? This is not the kind of problem where the platform should matter, only the compiler and the libc++ version. Let me try to reproduce locally.

Louis

> 
>> Unfortunately there is no way to debug CTAD deduction guides — the error
>> messages don't tell you which guides were considered as candidates or which
>> candidate was ultimately chosen.
>> 
>> –Arthur
> 
> -- 
> Best regards,
> Michał Górny
>