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 [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id B48263858D3C for ; Mon, 20 Sep 2021 19:32:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B48263858D3C Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-252-5bUJcBaFMvCCjaYocDADGQ-1; Mon, 20 Sep 2021 15:32:03 -0400 X-MC-Unique: 5bUJcBaFMvCCjaYocDADGQ-1 Received: by mail-qt1-f197.google.com with SMTP id x28-20020ac8701c000000b0029f4b940566so185848336qtm.19 for ; Mon, 20 Sep 2021 12:32:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=vWtPGkaQLqvbvTqIf6XWy1MaLrLiix2EzIVqMtc4XAw=; b=br55i3igMNj60k+CM7r3HtMxJseIqoyv7mAJNF1G4dnJ0etX2rnnxrgRrVQUZhD9FW xCth/hm1ezTH6inVgj/N088EL2HVecFrlI8YyyvYnAsySFPXXdWQ/rJhiNOJhpy2X/CN UWUpIMn9eB6a4Iw8/sNunIsyUXlfK6bi70d04dhMvXQZOqnP0dZUcUAoRnFfbdwOyfLY Ag57rEZm4oalc4cZI2adLzEnG8vP9m7of3a4NERkpFU8mszv5ELtk3phUiLAFsCP7zJg d5hn1Z5CErFe4QS2o4k/sYrWHc9lpVb4y5IDAHi06oWKCRA53N1smdo/jc+K8zlD8caq UMAg== X-Gm-Message-State: AOAM533TKv0SRfKIqXnyXB0onuotLO+45issPv+y8k88oMlPl2awafBP NnVjXHrF/uD5oFekFhLeXmk5KCCAtQi8CkOP3gHvhhHYVz1yEhmXKAjIyxESyH4fLH1LMF0CbDX h1UbjZfmassoJZZaKuA== X-Received: by 2002:a37:a146:: with SMTP id k67mr7901290qke.302.1632166323273; Mon, 20 Sep 2021 12:32:03 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwuftNdhLbhd/JvVoT3F+4UCUnWz57s0Ua8j9DF41AmTeEhVq0hBLLaNMb5hVk/PcqNAmx3IA== X-Received: by 2002:a37:a146:: with SMTP id k67mr7901267qke.302.1632166323010; Mon, 20 Sep 2021 12:32:03 -0700 (PDT) Received: from [192.168.1.130] (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id j26sm6543431qtr.53.2021.09.20.12.32.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Sep 2021 12:32:02 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Mon, 20 Sep 2021 15:32:01 -0400 (EDT) To: Jason Merrill cc: Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: consider built-in operator candidates first In-Reply-To: <19181760-52f1-babe-5b44-6471dea5b7bc@redhat.com> Message-ID: <13ea34a-deaf-a23f-ec27-863e19b618b@idea> References: <20210920164608.2740792-1-ppalka@redhat.com> <19181760-52f1-babe-5b44-6471dea5b7bc@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-16.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Sep 2021 19:32:07 -0000 On Mon, 20 Sep 2021, Jason Merrill wrote: > On 9/20/21 12:46, Patrick Palka wrote: > > During operator overload resolution, we currently consider non-member > > candidates before built-in candidates. This didn't make a difference > > before r12-3346, but after this change add_candidates will avoid > > computing excess argument conversions if we've already seen a strictly > > viable candidate, so it's better to consider built-in candidates first. > > Doesn't r12-3346 stop considering conversions after it sees a bad one, and > later return to the bad candidate if there is no strictly viable candidate? > How does this patch change that? Yes, but add_candidates also looks for a strictly viable candidate among the already-considered candidates in the 'candidates' list via the line: bool seen_strictly_viable = any_strictly_viable (*candidates); So by considering the built-in candidates first, the subsequent call to add_candidates that considers the non-member functions in will be aware of any (built-in) strictly viable candidate. > > Depending on the order of the candidates seems fragile. Yeah.. :/ I guess in general it'd be better to build up the entire overload set first and then call add_candidates exactly once (which would also make the perfect candidate optimization more consistent/effective). But I'm not sure if we can easily build up such an overload set in this case since built-in candidates are represented and handled differently than non-built-in candidates.. FWIW, although the test case added by this patch is contrived, this opportunity was found in the real world by instrumenting the 'bad_fns' mechanism added by r12-3346 to look for situations where we still end up using it (and thus end up redundantly considering some candidates twice), and this built-in operator situation was the most common in the codebases that I tested (although still quite rare in the codebases that I tested). > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > > trunk? > > > > gcc/cp/ChangeLog: > > > > * call.c (add_operator_candidates): Consider built-in operator > > candidates before considering non-member candidates. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/template/conv17.C: Extend test. > > --- > > gcc/cp/call.c | 13 +++++++------ > > gcc/testsuite/g++.dg/template/conv17.C | 7 +++++++ > > 2 files changed, 14 insertions(+), 6 deletions(-) > > > > diff --git a/gcc/cp/call.c b/gcc/cp/call.c > > index c5601d96ab8..c0da083758f 100644 > > --- a/gcc/cp/call.c > > +++ b/gcc/cp/call.c > > @@ -6321,7 +6321,6 @@ add_operator_candidates (z_candidate **candidates, > > vec *arglist, > > int flags, tsubst_flags_t complain) > > { > > - z_candidate *start_candidates = *candidates; > > bool ismodop = code2 != ERROR_MARK; > > tree fnname = ovl_op_identifier (ismodop, ismodop ? code2 : code); > > @@ -6333,6 +6332,12 @@ add_operator_candidates (z_candidate **candidates, > > if (rewritten && code != EQ_EXPR && code != SPACESHIP_EXPR) > > flags &= ~LOOKUP_REWRITTEN; > > + /* Add built-in candidates to the candidate set. The standard says to > > + rewrite built-in candidates, too, but there's no point. */ > > + if (!rewritten) > > + add_builtin_candidates (candidates, code, code2, fnname, arglist, > > + flags, complain); > > + > > bool memonly = false; > > switch (code) > > { > > @@ -6352,6 +6357,7 @@ add_operator_candidates (z_candidate **candidates, > > /* Add namespace-scope operators to the list of functions to > > consider. */ > > + z_candidate *start_candidates = *candidates; > > if (!memonly) > > { > > tree fns = lookup_name (fnname, LOOK_where::BLOCK_NAMESPACE); > > @@ -6423,11 +6429,6 @@ add_operator_candidates (z_candidate **candidates, > > if (!rewritten) > > { > > - /* The standard says to rewrite built-in candidates, too, > > - but there's no point. */ > > - add_builtin_candidates (candidates, code, code2, fnname, arglist, > > - flags, complain); > > - > > /* Maybe add C++20 rewritten comparison candidates. */ > > tree_code rewrite_code = ERROR_MARK; > > if (cxx_dialect >= cxx20 > > diff --git a/gcc/testsuite/g++.dg/template/conv17.C > > b/gcc/testsuite/g++.dg/template/conv17.C > > index f0f10f2ef4f..87ecefb8de3 100644 > > --- a/gcc/testsuite/g++.dg/template/conv17.C > > +++ b/gcc/testsuite/g++.dg/template/conv17.C > > @@ -61,3 +61,10 @@ concept E = requires { T().h(nullptr); }; > > static_assert(!E); > > #endif > > + > > +// Verify that the strictly viable built-in operator+ candidate precludes > > +// us from computing all argument conversions for the below non-strictly > > +// viable non-member candidate. > > +enum N { n }; > > +int operator+(N&, B); > > +int f = n + 42; > > > >