From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id F314E385703B for ; Thu, 5 Nov 2020 20:59:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F314E385703B Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-78--hT46xJcMJ-naGogfADDQw-1; Thu, 05 Nov 2020 15:59:26 -0500 X-MC-Unique: -hT46xJcMJ-naGogfADDQw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 88FC8E9EC1; Thu, 5 Nov 2020 20:59:25 +0000 (UTC) Received: from localhost (unknown [10.33.36.7]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1E6AA5C5DE; Thu, 5 Nov 2020 20:59:24 +0000 (UTC) Date: Thu, 5 Nov 2020 20:59:24 +0000 From: Jonathan Wakely To: Ville Voutilainen Cc: libstdc++ , gcc-patches List Subject: Re: [committed] libstdc++: Fix constraints on std::optional comparisons [PR 96269] Message-ID: <20201105205924.GJ503596@redhat.com> References: <20201105190941.GA3537038@redhat.com> <20201105195049.GI503596@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Nov 2020 20:59:30 -0000 On 05/11/20 22:12 +0200, Ville Voutilainen via Libstdc++ wrote: >On Thu, 5 Nov 2020 at 21:52, Jonathan Wakely via Libstdc++ > wrote: >> >> On 05/11/20 19:09 +0000, Jonathan Wakely wrote: >> >The relational operators for std::optional were using the wrong types >> >in the declval expressions used to constrain them. Instead of using >> >const lvalues they were using non-const rvalues, which meant that a type >> >might satisfy the constraints but then give an error when the function >> >body was instantiated. >> > >> >libstdc++-v3/ChangeLog: >> > >> > PR libstdc++/96269 >> > * include/std/optional (operator==, operator!=, operator<) >> > (operator>, operator<=, operator>=): Fix types used in >> > SFINAE constraints. >> > * testsuite/20_util/optional/relops/96269.cc: New test. >> > >> >Tested powerpc64le-linux. Committed to trunk. >> >> When concepts are supported we can make the alias templates >> __optional_eq_t et al use a requires-expression instead of SFINAE. >> This is potentially faster to compile, given expected improvements >> to C++20 compilers. >> >> I'm testing this patch. > >It concerns me that we'd have such conditional conceptifying just >because it's possibly faster to compile. >There's more types where we'd want to conditionally use concepts, but >perhaps we want to think a bit >more how to do that in our source code, rather than just make them >preprocessor-conditionals in the same >header. We might entertain conceptifying tuple, when concepts are >available. That may end up being >fairly verbose if it's done with preprocessor in . > >That's not to say that I'm objecting to this as such; I merely think >we want to be a bit careful with >conceptifying, and be rather instantly prepared to entertain doing it >with a slightly different source code >structure, which may involve splitting things across more files, which >would then involve adding more >headers that are installed. I agree. I only considered doing it here (and am posting it for comment rather than committing it right away) because we already have the alias helpers which are used in multiple places in the file. Without those, every relational operator would look like this if we used concepts conditionally: template constexpr auto operator==(const optional<_Tp>& __lhs, const optional<_Up>& __rhs) #if __cpp_lib_concepts requires requires(const _Tp __t, const _Up __u) { { *__lhs == *__rhs } -> convertible_to; } #else -> enable_if_t() == std::declval()), bool>, bool> #endif { return static_cast(__lhs) == static_cast(__rhs) && (!__lhs || *__lhs == *__rhs); } Or: template constexpr auto operator==(const optional<_Tp>& __lhs, const optional<_Up>& __rhs) #if __cpp_lib_concepts requires requires { *__lhs == *__rhs } -> convertible_to; } #else -> enable_if_t, bool> #endif { return static_cast(__lhs) == static_cast(__rhs) && (!__lhs || *__lhs == *__rhs); } Yuck. The second one is less verbose, but does overload resolution and type deduction for optional<_Tp>::operator* and optional<_Up>::operator*. That's unnecessary (and so compiles slower) because we know the result types are just const _Tp& and const _Up&, so the first version uses those types directly. Either way, having that #if-#else-#endif on every relational operator is NOT appealing. But since all the operators already use aliases like __optional_eq_t any changes are localized to those helpers. The actual rel ops themselves don't change. We definitely want to think about the trade offs though. So far we only use concepts in code that only has to compile as C++20, so we don't need to provide a non-concepts fallback for C++17, or where it's required for conformance (e.g. iterator_traits). That's definitely more palatable than preprocessor conditions choosing between two functionally equivalent ways to do the same thing.