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

[waffl3x <waffl3x@protonmail.com>]

On Friday, December 8th, 2023 at 12:25 PM, Jason Merrill <jason@redhat.com> wrote:
> 
> 
> On 12/6/23 02:33, waffl3x wrote:
> 
> > Here is the next version, it feels very close to finished. As before, I
> > haven't ran a bootstrap or the full testsuite yet but I did run the
> > explicit-obj tests which completed as expected.
> > 
> > There's a few test cases that still need to be written but more tests
> > can always be added. The behavior added by CWG2789 works in at least
> > one case, but I have not added tests for it yet. The test cases for
> > dependent lambda expressions need to be fleshed out more, but a few
> > temporary ones are included to demonstrate that they do work and that
> > the crash is fixed. Explicit object conversion functions work, but I
> > need to add fleshed out tests for them, explicit-obj-basic5.C has that
> > test.
> 
> > @@ -6586,6 +6586,17 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> args,
> > + / FIXME: I believe this will be bugged for xobj member functions,
> > + leaving this comment here to make sure we look into it
> > + at some point.
> > + Seeing this makes me want correspondence checking to be unified
> > + in one place though, not sure if this one needs to be different
> > + from other ones though.
> > + This function is only used here, but maybe we can use it in add
> > + method and move some of the logic out of there?
> 
> 
> fns_correspond absolutely needs updating to handle xob fns, and doing
> that by unifying it with add_method's calculation would be good.
> 
> > + Side note: CWG2586 might be relevant for this area in
> > + particular, perhaps we wait to see if it gets accepted first? */
> 
> 
> 2586 was accepted last year.
> 
> > @@ -12574,17 +12601,25 @@ cand_parms_match (z_candidate *c1, z_candidate c2)
> > fn1 = DECL_TEMPLATE_RESULT (t1);
> > fn2 = DECL_TEMPLATE_RESULT (t2);
> > }
> > + / The changes I made here might be stuff I was told not to worry about?
> > + I'm not really sure so I'm going to leave it in. */
> 
> 
> Good choice, this comment can go.
> 
> > tree parms1 = TYPE_ARG_TYPES (TREE_TYPE (fn1));
> > tree parms2 = TYPE_ARG_TYPES (TREE_TYPE (fn2));
> > if (DECL_FUNCTION_MEMBER_P (fn1)
> > && DECL_FUNCTION_MEMBER_P (fn2)
> > - && (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn1)
> > - != DECL_NONSTATIC_MEMBER_FUNCTION_P (fn2)))
> > + && (DECL_STATIC_FUNCTION_P (fn1)
> > + != DECL_STATIC_FUNCTION_P (fn2)))
> > {
> > /* Ignore 'this' when comparing the parameters of a static member
> > function with those of a non-static one. */
> > - parms1 = skip_artificial_parms_for (fn1, parms1);
> > - parms2 = skip_artificial_parms_for (fn2, parms2);
> > + auto skip_parms = [](tree fn, tree parms){
> > + if (DECL_XOBJ_MEMBER_FUNCTION_P (fn))
> > + return TREE_CHAIN (parms);
> > + else
> > + return skip_artificial_parms_for (fn, parms);
> > + };
> > + parms1 = skip_parms (fn1, parms1);
> > + parms2 = skip_parms (fn2, parms2);
> > }
> 
> 
> https://cplusplus.github.io/CWG/issues/2789.html fixes the handling of
> xobj fns here.

I have a minor concern here.
https://eel.is/c++draft/basic.scope.scope#3

Is it okay that CWG2789 has separate rules than the rules for
declarations? As far as I know there's nothing else that describes how
to handle the object parameters so it was my assumption that the rules
here are also used for that. I know at least one person has disagreed
with me about that so I'm not sure what is actually correct.

template <typename T = int>
struct S {
  constexpr void f() {}                       // #f1
  constexpr void f(this S&&) requires true {} // #f2

  constexpr void g() requires true {}         // #g1
  constexpr void g(this S&&) {}               // #g2
};

void test() {
  S<>{}.f();             // calls #?
  S<>{}.g();             // calls #?
}

But with the wording proposed by CWG2789, wouldn't this example would
be a problem? If we follow the standard to the letter, constraints
can't be applied here right?

I wouldn't be surprised if I'm missing something but I figured I ought
to raise it just in case. Obviously it should call #f2 and #g1 but I'm
pretty sure the current wording only allows these calls to be ambiguous.

> Your change does the right thing for comparing static and xobj, but
> doesn't handle comparing iobj and xobj; I think we want to share
> parameter comparison code with fns_correspond/add_method. Maybe
> parms_correspond?

Yeah, I'll see what I can put together. The only issue being what I
noted above, I'm not sure the rules are actually the same. I think they
should be, but my reading of things seems like it's not right now.

For the time being I'm going to assume things should work the way I
want them to, because I don't think the example I presented above
should be ambiguous.

> > @@ -8727,21 +8882,42 @@ resolve_address_of_overloaded_function (tree target_type,
> > /* Good, exactly one match. Now, convert it to the correct type. */
> > fn = TREE_PURPOSE (matches);
> > 
> > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
> > - && !(complain & tf_ptrmem_ok) && !flag_ms_extensions)
> > + if (DECL_OBJECT_MEMBER_FUNCTION_P (fn)
> > + && !(complain & tf_ptrmem_ok))
> > {
> > - static int explained;
> > -
> > - if (!(complain & tf_error))
> > + /* For iobj member functions, if if -fms_extensions was passed in, this
> > + is not an error, so we do nothing. It is still an error regardless
> > + for xobj member functions though, as it is a new feature we
> > + (hopefully) don't need to support the behavior. */
> 
> 
> Unfortunately, it seems that MSVC extended their weirdness to xobj fns,
> so -fms-extensions should as well.
> https://godbolt.org/z/nfvn64Kx5

Ugh, what a shame. We don't have to support it though do we? The
documentation for -fms-extensions seems to state that it's for
microsoft headers, if they aren't using xobj parameters in their
headers as of now, and I can't see them doing so for the near future,
it should be fine to leave this out still shouldn't it?

The code will be simpler if we can't though so I reckon it's win/win
whichever way we choose.

> > + /* I'm keeping it more basic for now. */
> 
> 
> OK, this comment can go.

Yeah, if I ever get comfortable with fixit's I'll do a loop through my
changes and see where I can enhance the ones already present and add
the ones that I wanted.

> > @@ -15502,9 +15627,10 @@ void
> > grok_special_member_properties (tree decl)
> > {
> > tree class_type;
> > -
> > + /* I believe we have to make some changes in here depending on the outcome
> > + of CWG2586. */
> 
> 
> As mentioned above, CWG2586 is resolved. Be sure to scroll down to the
> approved resolution, or refer to the working draft.
> https://cplusplus.github.io/CWG/issues/2586.html

Yeah I got a bit of a lecture on how to read these in IRC, I shouldn't
be making this mistake again. :'D

I'll look at supporting it, I imagine it shouldn't be too hard but I've
been wrong before.

> > @@ -11754,8 +11754,16 @@ cp_parser_lambda_declarator_opt (cp_parser* parser, tre
> > else if (cxx_dialect < cxx23)
> > omitted_parms_loc = cp_lexer_peek_token (parser->lexer)->location;
> > 
> > + /* Review note: I figured I might as well update the comments since I'm here.
> > + There are also some additions to the below. */
> 
> 
> Great, this comment can go.

Just noting, I'm also removing the old comment immediately following
this one as it is obsolete.

> > + /* [expr.prim.lambda.general-4]
> > + If the lambda-declarator contains an explicit object parameter
> > + ([dcl.fct]), then no lambda-specifier in the lambda-specifier-seq
> > + shall be mutable or static. /
> > + if (lambda_specs.storage_class == sc_mutable)
> > + {
> > + auto_diagnostic_group d;
> > + error_at (lambda_specs.locations[ds_storage_class],
> > + "%<mutable%> lambda specifier "
> > + "with explicit object parameter");
> > + / Tell the user how to do what they probably meant, maybe fixits
> > + would be apropriate later? */
> 
> 
> "appropriate"
> 
> > + if (!dependent_type_p (non_reference (param_type)))
> > + /* If we are given a non-dependent type we will have already given
> > + a diagnosis that the following would contradict with. */;
> 
> 
> Only if the lambda has captures, though?

Oops, yeah I agree.

> We could also change dependent_type_p to the more specific
> WILDCARD_TYPE_P, I think, both here and just above.

I don't understand the significance of this but I'll trust there is
one. Better to be consistent with your other changes anyway.

> > + else if (!TYPE_REF_P (param_type))
> > + inform (DECL_SOURCE_LOCATION (xobj_param),
> > + "the passed in closure object will not be mutated because "
> > + "it is taken by copy/move");
> 
> 
> "by value"
> 
> > @@ -3092,7 +3093,31 @@ finish_this_expr (void)
> > return rvalue (result);
> > 
> > tree fn = current_nonlambda_function ();
> > - if (fn && DECL_STATIC_FUNCTION_P (fn))
> > + if (fn && DECL_XOBJ_MEMBER_FUNCTION_P (fn))
> > + {
> > + auto_diagnostic_group d;
> > + error ("%<this%> is unavailable for explicit object member "
> > + "functions");
> > + /* Doing a fixit here is possible, but hard, might be worthwhile
> > + in the future. /
> > + tree xobj_parm = DECL_ARGUMENTS (fn);
> > + gcc_assert (xobj_parm);
> > + tree parm_name = DECL_NAME (xobj_parm);
> > + if (parm_name)
> > + inform (DECL_SOURCE_LOCATION (xobj_parm),
> > + "use explicit object parameter %qD instead",
> > + parm_name);
> > + else
> > + {
> > + gcc_rich_location xobj_loc (DECL_SOURCE_LOCATION (xobj_parm));
> > + / This doesn't work and I don't know why. I'll probably remove it
> > + before the final version. */
> > + xobj_loc.add_fixit_insert_after (" self");
> > + inform (DECL_SOURCE_LOCATION (xobj_parm),
> > + "name and use the explicit object parameter instead");
> 
> 
> Now seems to be the time to either fix or remove it.

I think I'll defer it to future work, I'll pull it from the next
version. Fixits are arcane, I'll have to do a deep dive on them to be
able to use them properly I think. I'll also get in touch with David
Malcolm as you suggested and see what I can learn about them from him.

> > @@ -12811,9 +12836,11 @@ capture_decltype (tree decl)
> > if (WILDCARD_TYPE_P (non_reference (obtype)))
> > /* We don't know what the eventual obtype quals will be. /
> > return NULL_TREE;
> > - int quals = cp_type_quals (type);
> > - if (INDIRECT_TYPE_P (obtype))
> > - quals |= cp_type_quals (TREE_TYPE (obtype));
> > + / This change possibly conflicts with another patch that is currently
> > + in flight. (Patrick Palka's PR83167 patch) I am just changing it for
> > + now to satisfy my tests. */
> > + int const quals = cp_type_quals (type)
> > + | cp_type_quals (TREE_TYPE (obtype));
> 
> 
> Doesn't TREE_TYPE (obtype) break for by-value xob? In that case I think
> we'd want cp_type_quals (obtype).

I feel like you're right, and I'm disappointed that I missed this case
in my tests. Indeed, I've checked and it ICEs for by-value xobj
parameters. I'll fix this and add the test case.

> > + The name "nonstatic" is no longer accurate with the addition of
> > + xobj member functions, should we change the name? */
> > 
> > bool
> > invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t complain)
> 
> 
> This rule still applies to all non-static member functions:
> https://eel.is/c++draft/expr.ref#6.3.2

Alright I see where my mistake is now, the comment regarding .* and ->*
threw me for a loop. Even so, I figured since we renamed
DECL_NONSTATIC_MEMBER_FUNCTION_P we might be better off renaming other
things too. So in this case, invalid_object_memfn_p instead of
invalid_nonstatic_memfn_p.

Obviously changing the errors and warnings is a different beast and
probably should be left alone.

> > @@ -2352,7 +2355,7 @@ invalid_nonstatic_memfn_p (location_t loc, tree expr, tsubst_flags_t complain)
> > if (is_overloaded_fn (expr) && !really_overloaded_fn (expr))
> > expr = get_first_fn (expr);
> > if (TREE_TYPE (expr)
> > - && DECL_NONSTATIC_MEMBER_FUNCTION_P (expr))
> > + && DECL_IOBJ_MEMBER_FUNCTION_P (expr))
> 
> 
> ...and so I think this should be OBJECT.
> https://godbolt.org/z/r6v4e1ePP

Yeah, I agree, definitely a mistake on my part.

Since we are here, following the same logic I presented above, can we
refuse to support -fms-extensions for xobj member functions? Until
microsoft is using xobj member functions in their headers we shouldn't
need to support it right?

Supporting their non-conformance going forward doesn't make sense to
me. Especially since the non-conformance with xobj member functions is
likely just because it was already this way. Until we know for sure
that they rely on this behavior for xobj member functions I think it
should remain off, it's a simple change to add it back in after all.

> Here's a patch to adjust all the remaining
> DECL_NONSTATIC_MEMBER_FUNCTION_P. With this patch -diagnostic7.C gets
> the old address of non-static diagnostic, I think correctly, so I'm not
> sure we still need the BASELINK change in cp_build_addr_expr_1?
> 
> Jason

Thanks, I'll evaluate the BASELINK change to see if you're right.
Hopefully you are, that would simplify things.

Is there anything special I need to do when adding this patch? I was
worried I'm supposed to maintain it's origin in it's own commit or
something. Well, with that said it's probably still something I should
learn to do anyway, I'm just trying really hard to put it off until the
patch is done.

Alex