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

Tue Jun 18 07:40:15 PDT 2019

Okay. I was just trying to give a heads up to Gauthier but I didn't act on it since it was late at night. I'll revert the change to <map>, but please be careful when you start working on the patch again as I'll revert additional commits that added XFAILs to the tests (specifically 782fff1). When you re-submit a patch, please make sure you include the XFAIL we had added.

Reverted here:
commit 1fab01f92bdb23bfce98ee1281fe7be10ec04373
Author: Louis Dionne <ldionne at apple.com>
Date:   Tue Jun 18 14:40:15 2019 +0000

    [libc++] Revert the addition of map/multimap CTAD

    This was found to be broken on Clang trunk. This is a revert of the
    following commits (the subsequent commits added XFAILs to the tests
    that were missing from the original submission):

        r362986: Implement deduction guides for map/multimap.
        r363014: Add some XFAILs
        r363097: Add more XFAILs
        r363197: Add even more XFAILs

    llvm-svn: 363688

Louis

> On Jun 17, 2019, at 22:43, Arthur O'Dwyer <arthur.j.odwyer at gmail.com> wrote:
> 
> Louis, in that case, I suggest you just revert the change to <map>.  That is, I see two possibilities here —
> 
> (1) Gauthier's change to the core compiler is broken in a subtle way that just happened to break our recent change to <map>, or
> (2) Arthur (me)'s change to <map> is broken in a subtle way that just happened to exploit the core compiler bug that Gauthier was fixing (which, incidentally, was reported by me)
> 
> — and I think (2) is vastly more likely than (1).
> 
> Unfortunately, we might need Richard Smith or Mike Spertus to explain exactly what's wrong with my change to <map>. :(
> 
> –Arthur
> 
> 
> On Mon, Jun 17, 2019 at 10:37 PM Louis Dionne <ldionne at apple.com <mailto:ldionne at apple.com>> wrote:
> + Gauthier
> 
> This is the culprit:
> 
> commit a27d26e290545ff0686c0d08975baa53b9a4878b
> Author: Gauthier Harnisch <tyker1 at outlook.com <mailto:tyker1 at outlook.com>>
> Date:   Fri Jun 14 08:40:04 2019 +0000
> 
>     [clang] Fixing incorrect implicit deduction guides (PR41549)
> 
>     Summary:
>     [[ https://bugs.llvm.org/show_bug.cgi?id=41549 <https://bugs.llvm.org/show_bug.cgi?id=41549> | bug report ]]
> 
>     Before this patch, implicit deduction guides were generated from the first declaration found by lookup.
>     With this patch implicit deduction guides are generated from the definition of the class template.
>     Also added test that was previously failing.
> 
>     Reviewers: rsmith
> 
>     Reviewed By: rsmith
> 
>     Subscribers: cfe-commits, Quuxplusone
> 
>     Tags: #clang
> 
>     Differential Revision: https://reviews.llvm.org/D63072 <https://reviews.llvm.org/D63072>
> 
>> On Jun 17, 2019, at 17:20, Michał Górny <mgorny at gentoo.org <mailto:mgorny at gentoo.org>> wrote:
>> 
>> On Mon, 2019-06-17 at 17:02 -0400, Louis Dionne wrote:
>>>> On Jun 17, 2019, at 15:46, Louis Dionne via libcxx-commits <libcxx-commits at lists.llvm.org <mailto:libcxx-commits at lists.llvm.org>> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Jun 17, 2019, at 14:50, Michał Górny <mgorny at gentoo.org <mailto: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 <mailto: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 <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.
>>> 
>>> Yes, I can reproduce locally with Clang trunk. I think this isn't showing up in the Linux bots because they use older compilers. This must be something that was introduced recently-ish. I'll bisect.
>>> 
>> 
>> Oh, and I was wondering if everybody had to hack in just-built clang
>> like I did ;-).  Thanks for looking into it.
>> 
>> 
>> -- 
>> Best regards,
>> Michał Górny
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20190618/a4037760/attachment-0001.html>