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.129.124]) by sourceware.org (Postfix) with ESMTPS id 378593858281 for ; Thu, 9 Nov 2023 23:42:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 378593858281 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 378593858281 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699573322; cv=none; b=koeZLLn187NLGR4d2Qj5P962Ou35ydT2OX0RpHhKzc37/EciY8vZD+oTJ/UrEnA5DB02fsNlgZsqhcK30PC9zLA1HJ7SSn+cCo/yYO2UEI9p9Arkk/oNjdVaSxxR2bkSXoJG3NvnhBZIY7aokxsSYWsBRIRTa7Uka8c4KPYV+mg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699573322; c=relaxed/simple; bh=bI32DtKtEe+o2osFFckcLXtfL+EKOlk9b+AeRXhu61Y=; h=DKIM-Signature:Message-ID:Date:MIME-Version:From:Subject:To; b=ls+ztAjq1TOrHpvoUbPds0W8RQmZYzHCNgZjWnbgp/5yG922MvmJxxV8IUevFTuNk0vtWFNx9LMcrk70eVriewAITPKMQbP3n9zLcJq1mdE+DrB2Ei6JRtibDysInTcqSUZASQJIb5xuG9tyc3sNfcxeZvTVUhbhXMaMi8KA7TE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699573319; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=B2b0BfJj0KcD0lK2U99sEHy4u1uV/EWnrFjcdnhQ4ws=; b=BEBCsKzqb7vy1qzbZChGolwFw/h7olFp1B0HRlGZCPLZe11T+7CNqstFEiiopsogDuXjsp I2d18XqKOvfZjaEl4ByFDhyylmAJ5KJilR4qCeVq6FtdeOORvWo8AwyXvEatPjW7yCDX+S XhrfYiTPs9Ly+r79NOp1HxqEQVeSLWY= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-191-HMiWx0RHO4-LiWkrHaH9Kg-1; Thu, 09 Nov 2023 18:41:58 -0500 X-MC-Unique: HMiWx0RHO4-LiWkrHaH9Kg-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-77a033cbd96so161000985a.2 for ; Thu, 09 Nov 2023 15:41:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699573318; x=1700178118; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=B2b0BfJj0KcD0lK2U99sEHy4u1uV/EWnrFjcdnhQ4ws=; b=i2aiPtTTt1HsMWQGUljl1nSsjwXP/oZ3nZfDY1sRDI8S9ymjj7uF/wofYkkKVMNMXo HClTZ164i9jAhZa9/SFQpjgXyiCxlo19F4G72Gp5/dn5zjrJCN5DgJE3Seon6tje7iXs qdXh/jtWwv9duzBYc8BP6AjT8QXeBWSUOBBgbVJjGufQJlC4i8kSVwafhlrrxPKxUa3E wGxbm7znZ3/4vap605dSyzcf87Ciog4AnBQAVEpy40363FKA0+FTnD4INVt/MDbw9MOI rMrEeD+Aym7xdYy1Px07o347PSG9g3h4o49dWXC9K7YLyT+otFB1WTfWww9TYRSoR/gN T/mQ== X-Gm-Message-State: AOJu0YwkzjdA9J9Ee2lPb8LYKTIC8c1tDbGN8UblNB9FEoNxzsZFHf61 BbyvhCLLRDfF8eycT9jHd6EZxBwvdj/UT5Ys0RsB98cU9dgUnWNS3TXe7bk3EPAr7wV2k8oGohR Hzs+asgWOo71W5GlBFQ== X-Received: by 2002:a05:620a:574:b0:778:9465:7407 with SMTP id p20-20020a05620a057400b0077894657407mr5425964qkp.71.1699573317943; Thu, 09 Nov 2023 15:41:57 -0800 (PST) X-Google-Smtp-Source: AGHT+IG/HBp+BLGJ7N02NCgLMa49e90fFlCwwR2UBZQtF0r/+Lt143V9qccSC0ux9SJcwCQ8uA0lKg== X-Received: by 2002:a05:620a:574:b0:778:9465:7407 with SMTP id p20-20020a05620a057400b0077894657407mr5425955qkp.71.1699573317584; Thu, 09 Nov 2023 15:41:57 -0800 (PST) Received: from [192.168.1.108] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id c5-20020a05620a164500b007671b599cf5sm287866qko.40.2023.11.09.15.41.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Nov 2023 15:41:57 -0800 (PST) Message-ID: <1cd7ddb9-2db2-4699-8670-6b2e6f423dc0@redhat.com> Date: Thu, 9 Nov 2023 18:41:56 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Jason Merrill Subject: Re: [PATCH v4 2/2] c++: Diagnostics for P0847R7 (Deducing this) [PR102609] To: waffl3x Cc: "gcc-patches@gcc.gnu.org" References: In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.6 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_H4,RCVD_IN_MSPIKE_WL,RCVD_IN_SORBS_WEB,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/5/23 10:06, waffl3x wrote: > I had wanted to write about some of my frustrations with trying to > write a test for virtual specifiers and errors/warnings for > shadowing/overloading virtual functions, but I am a bit too tired at > the moment and I don't want to delay getting this up for another night. > In short, the standard does not properly specify the criteria for > overriding functions, which leaves a lot of ambiguity in how exactly we > should be handling these cases. Agreed, this issue came up in the C++ committee meeting today. See https://cplusplus.github.io/CWG/issues/2553.html https://cplusplus.github.io/CWG/issues/2554.html for draft changes to clarify some of these issues. > The standard also really poorly > specifies things related to the implicit object parameter and implicit > object argument which also causes some trouble. Anyhow, for the time > being I am not including my test for diagnostics related to a virtual > specifier on xobj member functions. I can't get it to a point I am > happy with it and I think there will need to be some discussion on how > exactly we want to handle that. The discussion might be easier with the testcase to refer to? > I was fairly lazy with the changelog and commit message in this patch > as I expect to need to do another round on this patch before it can be > accepted. One specific question I have is whether I should be listing > out all the diagnostics that were added to a function. For the cases > where there were only one diagnostic added I stated it, but for > grokdeclarator which has the majority of the diagnostics I did not. I > welcome input here, really I request it, because the changelogs are > still fairly difficult for me to write. Hell, the commit messages are > hard to write, I feel I went overboard on the first patch but I guess > it's a fairly large patch so maybe it's alright? Again, I am looking > for feedback here if anyone is willing to provide it. ChangeLog entries are very brief summaries of the changes, there's absolutely no need to enumerate multiple diagnostics. If someone wants more detail they can look at the patch. > + if (xobj_func_p && (quals || rqual)) > + inform (DECL_SOURCE_LOCATION (DECL_ARGUMENTS (decl)), > + "explicit object parameter declared here"); When you add an inform after a diagnostic, you also need to add an auto_diagnostic_group declaration before the error so that they get grouped together for JSON/SARIF diagnostic output. This applies to a lot of the diagnostics in the patch. > + pedwarn(DECL_SOURCE_LOCATION (xobj_parm), OPT_Wc__23_extensions, Missing space before ( > + /* If */ I think this comment doesn't add much. :) > + else if (declarator->declarator->kind == cdk_ptrmem) > + error_at (DECL_SOURCE_LOCATION (xobj_parm), > + "a member function pointer type " > + "cannot have an explicit object parameter"); Let's say "a pointer to member function type " > + /* Ideally we should synthesize the correct syntax > + for the user, perhaps this could be added later. */ Should be pretty simple to produce an add_fixit_remove() for the 'this' token here? > + /* Free function case, > + surely there is a better way to identify it? */ Move these diagnostics down past where ctype gets set? > + else if (decl_context == NORMAL > + && (in_namespace > + || !declarator->declarator->u.id.qualifying_scope)) > + error_at (DECL_SOURCE_LOCATION (xobj_parm), > + "a free function cannot have " > + "an explicit object parameter"); Let's say "non-member function". > + /* Ideally we synthesize a full rewrite, at the moment > + there are issues with it though. > + It rewrites "f(S this & s)" correctly, > + but fails to rewrite "f(const this S s)" correctly. > + It also does not handle "f(S& this s)" correctly at all. David Malcolm would be the one to ask for advice about fixit tricks, if you want. > + It's also possible we want to wait and see if the parm > + could even be a valid xobj parm as it might be confusing > + to the user to see an error, fix it, and then see another > + error for something new. I don't see how that applies here; we don't bail out after this error, so we should continue to give any other needed errors. > + /* If default_argument is non-null token should always be the > + the location of the `=' token, this is brittle code though > + and should be rectified in the future. */ It would be easy enough to add an eq_token variable? > + /* I can imagine doing a fixit here, suggesting replacing > + this / *this / this-> with &name / name / "name." but it would be > + very difficult to get it perfect and I've been advised against > + making imperfect fixits. > + Perhaps it would be as simple as the replacements listed, > + even if one is move'ing/forward'ing, if the replacement is just > + done in the same place, it will be exactly what the user wants? > + Even if this is true though, there's still a problem of getting the > + context of the expression to find which tokens to replace. > + I would really like for this to be possible though. > + I will decide whether or not to persue this after review. */ You could pass the location of 'this' from the parser to finish_this_expr and always replace it with (&name)? > + tree xobj_parm = FUNCTION_DECL_CHECK (fn)->function_decl.arguments; DECL_ARGUMENTS (fn) > + tree parm_name = DECL_MINIMAL_CHECK (xobj_parm)->decl_minimal.name; DECL_NAME (xobj_parm) > +++ b/gcc/testsuite/g++.dg/cpp23/explicit-obj-cxx-dialect-B.C > @@ -0,0 +1,7 @@ > +// P0847R7 > +// { dg-do compile { target c++20_down } } > +// { dg-options "-pedantic-errors" } -pedantic-errors is the default in the testsuite if there is no dg-options. > +++ b/gcc/testsuite/g++.dg/cpp23/explicit-obj-cxx-dialect-C.C > @@ -0,0 +1,7 @@ > +// P0847R7 > +// { dg-do compile { target c++20_down } } > +// { dg-options "-Wno-error=pedantic" } This has the same effect as { dg-options "" } since any dg-options replaces the default -pedantic-errors. Also, -Wno-error=pedantic does not turn off -pedantic-errors for -Wc++23-extensions. > + func_ptr_type xobj_mem_f_ptr = &xobj_member_f; // { dg-error "undetermined error" "disallowing address of unqualified explicit object member function is not implemented yet" { xfail *-*-* } } This would be complaining in cp_build_addr_expr_1 if you end up taking the address of an xobj fn without arg being an OFFSET_REF (with PTRMEM_OK_P). > + void f1() const; // { dg-note "previous declaration" "detecting redeclarations of iobj member functions without a ref qualifier as xobj member functions is known to be broken" { xfail *-*-* } } > + void f1(this S1 const&); // { dg-error "cannot be overloaded with" "detecting redeclarations of iobj member functions without a ref qualifier as xobj member functions is known to be broken" { xfail *-*-* } } In add_method, the code > /* Compare the quals on the 'this' parm. Don't compare > the whole types, as used functions are treated as > coming from the using class in overload resolution. */ > if (! DECL_STATIC_FUNCTION_P (fn) > && ! DECL_STATIC_FUNCTION_P (method) > /* Either both or neither need to be ref-qualified for > differing quals to allow overloading. */ > && (FUNCTION_REF_QUALIFIED (fn_type) > == FUNCTION_REF_QUALIFIED (method_type)) > && (type_memfn_quals (fn_type) != type_memfn_quals (method_type) > || type_memfn_rqual (fn_type) != type_memfn_rqual (method_type))) > continue; needs adjustment for the new "corresponding object parameters". Jason