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

[Arthur O'Dwyer via libcxx-commits]

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> wrote:

> + Gauthier
>
> This is the culprit:
>
> commit a27d26e290545ff0686c0d08975baa53b9a4878b
> Author: Gauthier Harnisch <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 | 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
>
> On Jun 17, 2019, at 17:20, Michał Górny <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> wrote:
>
>
>
> 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.
>
>
> 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/20190617/079ab811/attachment-0001.html>