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 2B50F3858D33 for ; Wed, 22 Nov 2023 03:22:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2B50F3858D33 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 2B50F3858D33 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=1700623360; cv=none; b=b2MVrfJ0KNMPAl9/YQKF3G8LlqmYLWHA+g6BOyeOQJ4XzKzR7yiCgCO+umsiAjEsE5vvb7VzeBzxxu7u4RjzDJMqVCKD6a4Fil+jwo1LzIWCPbOMJIEmIvfve2WM7I2okYWPPtPxgJyhgGIAHRt9Bb6kwoP/Foh5MNWLpMMRrzc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700623360; c=relaxed/simple; bh=OwLqZIxmqlEs19Xcnjqd0o8pVL1+eTiypdajCudr7OI=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=mRF2hd7p/K5HaUTDJ7CbvV+iXAs8HusIv4rQu74ig9ZyI/Z197aiKAzBis7TGC5zzBLk1dS3/SgtJTyLXdID15S48j54/TGuuZdBc/1OewhyYiu85T3j127xvnss/gSkxsNMvtc8SF2MiCi0fWUBRFq3MEoqs//Ro/ahkE3Vg7Q= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1700623357; 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=0qWy3iXse/O6F6iEFHmFGsRPaFa6KtPHjKlKVDXmdqM=; b=P0wkaU40RF53hvlYkc3bT209yc5Ab7s//wvhYuDam3vajLmC7M93K28QkyMT2ce6M737eV SgExBKYRcQTSoYI07e1nQ/JnsnuwNMH8Rs4K0DdN0AKkrBgDDtJkn/XTQukMLPywt1KHk5 lREFNpjoTzPDkCvsI1FkfUbYcG0+8zo= Received: from mail-yw1-f197.google.com (mail-yw1-f197.google.com [209.85.128.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-685-HOU8u9btO0a48HO7W24ejg-1; Tue, 21 Nov 2023 22:22:36 -0500 X-MC-Unique: HOU8u9btO0a48HO7W24ejg-1 Received: by mail-yw1-f197.google.com with SMTP id 00721157ae682-5aecf6e30e9so92414087b3.1 for ; Tue, 21 Nov 2023 19:22:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700623355; x=1701228155; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=0qWy3iXse/O6F6iEFHmFGsRPaFa6KtPHjKlKVDXmdqM=; b=L0vtlp9ZA/cnBheifE0OYyVNLgUC3KOlUWg0BhVZY7mXWcWXoAKy9l12KCClwJQkzo T9t+YJ/FuxnUJDTm+oroaCDXyMt04LEv/2Su1j+6trnawTfP50I0dcZIsdi7nfwPqL5V o+XEqO1fnf1hNvA+5oujIYDFez0NvyA9t++Qy9ASu1f8MZOhveduy9JpsHt/wOgv4xby hRD+hwZcgsTxwAW2lxihfsFccZPyxJwx/9VPJGSbJNtRY/fV7BXSnjRQyUuJTpM2/nZW N9yg8+5ihPP4DYkmS0JWeMq7xGicQpzRCHdNEF3ArjQfjqFAim2dzj+EgQKSI6XAuCLc rifQ== X-Gm-Message-State: AOJu0YzIqsWHJ+u775r1Rx6+PRi7Wk03XFCJi9wo4aXDnKs2qZGSUBeG Mlr6USg3kiArLsgBk6RyQ5yKL7agLboo5W8uuEfk0h1c3GZj7ijcFF162cmrJZEcjj83PuBZg8p v5xZcnB2ekoJDU7H3yFYeQltL+A== X-Received: by 2002:a81:5f07:0:b0:59b:ec10:915e with SMTP id t7-20020a815f07000000b0059bec10915emr996927ywb.30.1700623355311; Tue, 21 Nov 2023 19:22:35 -0800 (PST) X-Google-Smtp-Source: AGHT+IExyFtidnUaJ8mKf0JKlNiHa6VHYUfH0BcurFzpaqRaA9Tto6rtA3w6x+gB+dhgSLXewmzZMQ== X-Received: by 2002:a81:5f07:0:b0:59b:ec10:915e with SMTP id t7-20020a815f07000000b0059bec10915emr996920ywb.30.1700623354873; Tue, 21 Nov 2023 19:22:34 -0800 (PST) Received: from [192.168.1.145] (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 15-20020a05620a04cf00b00767da10efb6sm4148042qks.97.2023.11.21.19.22.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 21 Nov 2023 19:22:34 -0800 (PST) Message-ID: Date: Tue, 21 Nov 2023 22:22:33 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 1/1] c++: Initial support for P0847R7 (Deducing This) [PR102609] To: waffl3x Cc: "gcc-patches@gcc.gnu.org" References: <9890a007-755e-41e2-bc33-37ebf0755435@redhat.com> <1MdaTybBd_o4uw-Gb23fYyd5GNz7qFqoSe_f5h90LY_BdzM2ge2qPSyCuiCLYoYcZSjmVv13fw1LmjQC_M2L8raS1fydY5pEJ_vwvv5Z-0k=@protonmail.com> From: Jason Merrill 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=-6.0 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,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham 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/21/23 08:04, waffl3x wrote: > Bootstrapped and tested on x86_64-linux with no regressions. > > Hopefully this patch is legible enough for reviewing purposes, I've not > been feeling the greatest so it was a task to get this finished. > Tomorrow I will look at putting the diagnostics in > start_preparsed_function and also fixing up anything else. > > To reiterate in case it wasn't abundantly clear by the barren changelog > and commit message, this version is not intended as the final revision. > > Handling re-declarations was kind of nightmarish, so the comments in > there are lengthy, but I am fairly certain I implemented them correctly. > > I am going to get some sleep now, hopefully I will feel better tomorrow > and be ready to polish off the patch. Thanks for the patience. Great! > I stared at start_preparsed_function for a long while and couldn't > figure out where to start off at. So for now the error handling is > split up between instantiate_body and cp_parser_lambda_declarator_opt. > The latter is super not correct but I've been stuck on this for a long > time now though so I wanted to actually get something that works and > then try to make it better. I see what you mean, instantiate body isn't prepared for start_preparsed_function to fail. It's ok to handle this in two places. Though actually, instantiate_body is too late for it to fail; I think for the template case it should fail in tsubst_lambda_expr, before we even start to consider the body. Incidentally, I notice this code in tsubst_function_decl seems to need adjusting for xobj: tree parms = DECL_ARGUMENTS (t); if (closure && !DECL_STATIC_FUNCTION_P (t)) parms = DECL_CHAIN (parms); parms = tsubst (parms, args, complain, t); for (tree parm = parms; parm; parm = DECL_CHAIN (parm)) DECL_CONTEXT (parm) = r; if (closure && !DECL_STATIC_FUNCTION_P (t)) ... and this in tsubst_lambda_expr that assumes iobj: /* Fix the type of 'this'. */ fntype = build_memfn_type (fntype, type, type_memfn_quals (fntype), type_memfn_rqual (fntype)); This also seems like the place to check for unrelated type. > /* Nonzero for FUNCTION_DECL means that this decl is a non-static > - member function. */ > + member function, use DECL_IOBJ_MEMBER_FUNC_P instead. */ > #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \ > (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE) > > +/* Nonzero for FUNCTION_DECL means that this decl is an implicit object > + member function. */ > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \ > + (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE) I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than add a new, equivalent one. And then go through all the current uses of the old macro to decide whether they mean IOBJ or OBJECT. > - (static or non-static). */ > + (static or object). */ Let's leave this comment as it was. > + auto handle_arg = [fn, flags, complain](tree type, > + tree arg, > + int const param_index, > + conversion *conv, > + bool const conversion_warning) Let's move the conversion_warning logic into the handle_arg lambda rather than have it as a parameter. Yes, we don't need it for the xobj parm, but I think it's cleaner to have less in the loop. Also, let's move handle_arg after the iobj 'this' handling so it's closer to the uses. For which the 'else xobj' needs to drop the 'else', or change to 'if (first_arg)'. > + /* We currently handle for the case where first_arg is NULL_TREE > + in the future this should be changed and the assert reactivated. */ > + #if 0 > + gcc_assert (first_arg); > + #endif Let's leave this out. > + val = handle_arg(TREE_VALUE (parm), Missing space before (. > - if (null_node_p (arg) > - && DECL_TEMPLATE_INFO (fn) > - && cand->template_decl > - && !cand->explicit_targs) > - conversion_warning = false; > - > + auto determine_conversion_warning = [&]() > + { > + return !(null_node_p (current_arg) > + && DECL_TEMPLATE_INFO (fn) > + && cand->template_decl > + && !cand->explicit_targs); > + }; I don't think this needs to be a lambda. > + declaration, then the type of it's object parameter is still ... > + but due to [basic.scope.scope.3] we need to ignore it's ... > + /* Since a valid xobj parm has it's purpose cleared in find_xobj_parm "its" > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn) > - && !(complain & tf_ptrmem_ok) && !flag_ms_extensions) > + if (DECL_OBJECT_MEMBER_FUNC_P (fn) > + && !(complain & tf_ptrmem_ok)) The following code (not quoted) could share more code between the cases if we check !(flag_ms_extensions && DECL_IOBJ_MEMBER_FUNC_P (fn)) here? > + else if (declarator->declarator->kind == cdk_pointer) > + error_at (DECL_SOURCE_LOCATION (xobj_parm), > + /* "a pointer to function type cannot "? */ > + "a function pointer type cannot " > + "have an explicit object parameter"); "pointer to function type", yes. > + /* The locations being used here are probably not correct. */ Why not? Let's clear xobj_parm after giving an error in the TYPENAME case > if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl)) > || DECL_STATIC_FUNCTION_P (decl)) I think this can just be if (DECL_OBJECT_MEMBER_FUNC_P (decl)). > if (TREE_CODE (fntype) == METHOD_TYPE) > ctype = TYPE_METHOD_BASETYPE (fntype); > + else if (DECL_XOBJ_MEMBER_FUNC_P (decl1)) > + ctype = DECL_CONTEXT (decl1); All of this can be if (DECL_CLASS_SCOPE_P (decl1)) ctype = DECL_CONTEXT (decl1); I think I'm going to go ahead and clean that up now. > + /* We don't need deal with 'this' or vtable for xobj member functions. */ > + if (ctype && !doing_friend && > + !(DECL_STATIC_FUNCTION_P (decl1) || DECL_XOBJ_MEMBER_FUNC_P (decl1))) And this can be if (DECL_IOBJ_MEMBER_FUNC_P (decl1)) > + if (INDIRECT_TYPE_P (TREE_TYPE (object))) > + /* The proxy variable forwards to the capture field. */ This comment should stay outside the if. > + /* This might not be the most appropriate place for this, but I'm going > + to hold back on changing it for the time being since we are short on > + time. It's not really a parser error, it's more a semantic error. > + But this is where the other errors were so... */ It's OK to give semantic errors in the parser if they are always detectable at parse time (before template instantiation). Though maybe we could contain the xobj vs mutable and xobj vs static diagnostics in this block: > + if (xobj_param) > + { > + quals = TYPE_UNQUALIFIED; > + if (TYPE_REF_P (xobj_param) > + && !(cp_type_quals (TREE_TYPE (xobj_param)) & TYPE_QUAL_CONST)) > + LAMBDA_EXPR_MUTABLE_P (lambda_expr) = 1; > + } rather than separately in the mutable/static blocks? But the way you have it is fine, too. > + inform (DECL_SOURCE_LOCATION(xobj_param), missing space before (xobj_param), repeated several times in the rest of the function. > + /* Error reporting here is a little awkward, if the type of the > + object parameter is deduced, we should tell them the lambda > + is effectively already const, or to make the param const if it is > + not, but if it is deduced and taken by value shouldn't we say > + that it's taken by copy and won't mutate? > + Seems right to me, but it's a little strange. */ I think just omit the inform if dependent_type_p. > - if (!LAMBDA_EXPR_STATIC_P (lambda_expr)) > + if (!LAMBDA_EXPR_STATIC_P (lambda_expr) > + && !DECL_XOBJ_MEMBER_FUNC_P (fco)) if (DECL_IOBJ_MEMBER_FUNC_P (fco)) > + /* Since a lambda's type is anonymous, we can assume an explicit object > + parameter is unrelated... at least I believe so. I can think of a > + paradoxical case that almost works, where a class is forward declared > + and then defined deriving from a lambda that has an explicit object > + parameter of that class type, but the only way that can work is with > + decltype... okay yeah I think it can work for a struct defined in > + block scope, but I'm leaving this as is until I can confirm my > + hypothesis. */ Like this? template struct A: T { }; template T f() { int x = 42; auto f1 = [x](this A&& self) { return x; }; return A{f1}(); } int main() { return f(); } When I disable the error this crashes in register_parameter_specializations > + if (!TEMPLATE_PARM_P (non_reference (TREE_TYPE (xobj_param)))) ...so let's just check dependent_type_p here. > + error ("% is unavailable for explicit object member " > + "functions"); > + /* 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. */ I still think (&self) will always be a correct replacement for 'this', but unlikely to be the way they actually want their code to look. To detect this-> or *this should be doable in the parser (where you can look forward and back in the token stream) but we don't have access to that here in finish_this_expr. > + /* Pull out the function_decl for a single xobj member function, > + similar to the BASELINK case above. If we did the same optimization > + as we do for single static member functions (passing in a BASELINK) > + then it could be handled the same too. */ This comment seems out of date given that the BASELINK handling just gives an error. We can't use the same handling as static member functions because we need the SCOPE_REF to tell us that it was properly qualified. Jason