Re: [PATCH v5 1/1] c++: Initial support for P0847R7 (Deducing This) [PR102609]

[waffl3x <waffl3x@protonmail.com>]

On Tuesday, November 21st, 2023 at 8:22 PM, Jason Merrill <jason@redhat.com> wrote:


>
>
> 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));

Originally I was going to say this doesn't look like a problem in
tsubst_lambda_expr, but after looking at tsubst_function_decl I'm
thinking it might be the source of some trouble. If it really was
causing problems I would think it would be working much worse than it
currently is, but it does feel like it might be the actual source of
the bug I was chasing yesterday. Assigning to a capture with a deduced
const xobj parameter is not being rejected right now, this might be
causing it. I'll look more thoroughly today.

> This also seems like the place to check for unrelated type.

It does feel that way, I agree.

> > /* 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.

I figure it would be easier to make that transition if there's a clear
line between old versus new. To be clear, my intention is for the old
macro to be removed once all the uses of it are changed over to the new
macro. I can still remove it for the patch if you like but having both
and removing the old one later seems better to me.

> > - (static or non-static). */
> > + (static or object). */
>
>
> Let's leave this comment as it was.

Okay.

> > + 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.

I would argue that it's cleaner to have the lambda be concise, but I'll
make this change.

> 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)'.

Agreed, I didn't like how far away it was.

> > + /* 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.

Alright.

> > + 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.

Yeah probably a little unnecessary, I'll switch it back.

> > + 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"

TIL while we use an apostrophe for possessive nouns, we don't use one
for the posessive its. English!

> > - 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?

Yeah I didn't like it very much when I wrote it, but I was too tired to
reliably merge things together so I just wanted it to work. I will
screw around with it more today.

> > + 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?

I threw them in just so I could call inform, but it doesn't feel like
the messages should be pointing at the parameter, but rather at the
full type declaration. When I put those locations in I wasn't sure how
to get the full declaration location, and I'm still not 100% confident
in how to do it, so I just threw them in and moved on.

> Let's clear xobj_parm after giving an error in the TYPENAME case

I don't like the spirit of this very much, whats your reasoning for
this? We're nearly at the end of the scope where it is last used, I
think it would be more unclear if we suddenly set it to NULL_TREE near
the end. It raises the question of whether that assignment actually
does anything, or if we are just trying to indicate that it isn't being
used anymore, but I already made sure to declare it in the deepest
scope possible. That much should be sufficient for indicating it's
usage, no?

> > 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)).

Alright, and going forward I'll try to make more changes that are
consistent with this one. With that said I'm not sure it can, but I'll
take a close look and if you're right I'll make that change.

> > 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.

Sounds good to me, a lot of this stuff needs small cleanups and I'm
just concerned about making them too much.

> > + /* 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).

Okay, it's good to hear a definite answer to that, I'll keep it in mind.

> 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.

Yeah I don't like the code very much, I am happy to refactor it, it's
just another case of tired coding so I didn't move things around too
much.

> > + inform (DECL_SOURCE_LOCATION(xobj_param),
>
>
> missing space before (xobj_param), repeated several times in the rest of
> the function.

Yeah tired coding. :)
I'll be sure to go over everything with a fine tooth comb again before
the final version of the patch.

> > + /* 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.

Maybe I don't understand what a dependent type is as well as I thought,
but doesn't this defeat every useful case? The most common being an
xobj parameter of lambda type, which will always be deduced. Unless a
template parameter does not count as a dependent type, which is not
something I've ever thought about before.

It does need to be better though, so I'll work on it and I'll keep what
you said in mind.

> > - 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 <class T> struct A: T { };
>
> template <class T> T f()
>
> {
> int x = 42;
> auto f1 = [x]<typename U>(this A<U>&& self) { return x; };
>
> return A<decltype(f1)>{f1}();
>
> }
> int main() { return f<int>(); }

Oh boy, I had a feeling I was wrong. I was really hoping I was right
but here it is, that definitely counts. I had tried something like this
but I decided that it wasn't possible, I completely forgot about
template argument deduction and passing it in like this.

>
> 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.

I think this jogs my memory on what a dependent type is. I believe I
see how this would work now.

Mildly related, a lot of the stuff I hacked together with multiple
levels of accessing macros and predicates was due to not being able to
find a solution for what I needed. I think we would highly benefit from
better documentation of the accessors and predicates. I believe I've
seen some that appear to be duplicates, and some where they don't
appear to be implemented properly or match their description. If there
is such a document please direct me to it as I have spent an hour or so
each time I stumble on one of these problems.

In the section I wrote in add_method I spent I think about 3 hours
trying to figure out what combination of predicates and accessing
macros was correct for what I was trying to do. It gets pretty
convoluted to traverse, especially when the implementations of them are
rather involved. Finding non_reference was a big help for example, and
I stumbled across it by pure luck.

If we don't have this kind of documentation anywhere yet, I'm happy to
work on it next. I've spent a lot of time looking at it all so I feel
like I have a DECENT grasp on some of it. I think it would greatly
enhance the status quo, and might even help us figure out what needs to
be cleaned up, removed, replaced, etc.

Side side note, if said document does exist already, I either need to
learn how to search the wiki more effectively or it needs some sort of
improvement.

> > + error ("%<this%> 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.

Yeah, that is the issue. At the moment I'm putting any fixit stuff that
I want to do on the backburner. I've come across a few more I wanted to
do but, I've just come to the conclusion that learning how to do fixits
properly will be something I'll need to sit down and study on its own.
They are really finicky, the last one I tried to do just doesn't seem
to show up at all and I don't really know why.

Regarding (&self), yeah it will always be valid, it's just like you
said it's unlikely to be what the user means. I don't want to put a
half solution in here, I'll just delay the full solution.

> > + /* 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

I can revise how the comment reads, especially since as you've pointed
out introducing the baselink optimization to non-overloaded xobj member
functions would cause more problems than benefits. What I meant by the
comment was that it just simply pulls the function_decl out and puts it
in arg, then lets everything else after the switch case handle it. It
definitely isn't clear the way it's written right now, I will revise it.

I mostly got tests written yesterday, not a whole lot else done. I
found that one of my comments was false specifically regarding the
change in build_capture_proxy.

+  /* Making this conditional prevents the ICE, but does not actually work
+     correctly.  */

So funny enough, the comment seems to be false for the version of the
patch I submitted. I removed the change I made in
finish_non_static_data_member before submitting the patch, but didn't
realize that the change I made in there was what actually caused the
problem I was referencing. It wasn't until yesterday morning that I
realized that the behavior was actually correct. I also blame GDB for
acting funky, for some reason even in a build with optimizations off,
assignments to variables were appearing late. I blame GDB but it really
might not be its fault. Regardless, it threw me off quite a bit a few
times I think, I only noticed it when I started comparing return values
from functions (with the finish command) and what they were being
assigned to. It seems to happen when assigning to a variable that was
declared but not initialized.

Rambling aside, the point is, that simple change you suggested in
build_capture_proxy does appear to have been sufficient for fixing the
by-value xobj parameter case. The next patch will have tests to
demonstrate these cases. Realistically I should have had the tests done
for this version but, I just really couldn't manage it. With how much
feedback you gave I think I made the right call getting it sent faster
rather than waiting for all these things.

I will attempt to get another version finished today. I have a feeling
you were right about tsubst_lambda_expr and tsubst_function_decl so
hopefully that will expedite things. As noted, the next version will
include more tests so there will be less mystery in what works and what
doesn't.

Alex