Re: [PATCH v4 1/2] c++: Initial support for P0847R7 (Deducing this) [PR102609]

[waffl3x <waffl3x@protonmail.com>]

Ahh, I should have updated my progress last night after all, it would
have saved us some time. Regardless, it's nice to see we independently
came to the same conclusions.

Side note, would you prefer I compile the lambda and by-value fixes
into a new version of this patch? Or as a separate patch? Originally I
had planned to put it in another patch, but I identified that the code
I wrote in build_over_call was kind of fundamentally broken and it was
almost merely coincidence that it worked at all. In light of this and
your comments (which I've skimmed, I will respond directly below) I
think I should just revise this patch with everything else.


On Thursday, November 9th, 2023 at 2:53 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/5/23 10:06, waffl3x wrote:
> 
> > Bootstrapped and tested on x86_64-linux with no regressions.
> > 
> > I originally threw this e-mail together last night, but threw in the
> > towel when I thought I saw tests failing and went to sleep. I did a
> > proper bootstrap and comparison and whatnot and found that there were
> > thankfully no regressions.
> > 
> > Anyhow, the first patch feels ready for trunk, the second needs at
> > least one review, I'll write more on that in the second e-mail though.
> > I put quite a lot into the commit message, in hindsight I think I may
> > have gone overboard, but that isn't something I'm going to rewrite at
> > the moment. I really want to get these patches up for review so they
> > can be finalized.
> > 
> > I'm also including my usual musings on things that came up as I was
> > polishing off the patches. I reckon some of them aren't all that
> > important right now but I would rather toss them in here than forget
> > about them.
> > 
> > I'm starting to think that we should have a general macro that
> > indicates whether an implicit object argument should be passed in the
> > call. It might be more clear than what is currently present. I've also
> > noticed that there's a fair amount of places where instead of using
> > DECL_NONSTATIC_MEMBER_FUNCTION_P the code checks if tree_code of the
> > type is a METHOD_TYPE, which is exactly what the aforementioned macro
> > does.
> 
> 
> Agreed.
> 
> > In build_min_non_dep_op_overload I reversed the branches of a condition
> > because it made more sense with METHOD_TYPE first so it doesn't have to
> > take xobj member functions into account on both branches. I am slightly
> > concerned that flipping the branch around might have consequences,
> > hence why I am mentioning it. Realistically I think it's probably fine
> > though.
> 
> 
> Agreed.

Great, I was definitely concerned about this.
 
> > BTW let me know if there's anything you would prefer to be done
> > differently in the changelog, I am still having trouble writing them
> > and I'm usually uncertain if I'm writing them properly.
> 
> > (DECL_FUNCTION_XOBJ_FLAG): Define.
> 
> 
> This is usually "New macro" or just "New".
> 
> > * decl.cc (grokfndecl): New param XOBJ_FUNC_P, for xobj member
> > functions set DECL_FUNCTION_XOBJ_FLAG and don't set
> > DECL_STATIC_FUNCTION_P.
> > (grokdeclarator): Check for xobj param, clear it's purpose and set
> > is_xobj_member_function if it is present. When flag set, don't change
> > type to METHOD_TYPE, keep it as FUNCTION_TYPE.
> > Adjust call to grokfndecl, pass is_xobj_member_function.
> 
> 
> These could be less verbose; for grokfndecl it makes sense to mention
> the new parameter, but otherwise just saying "handle explicit object
> member functions" is enough.

Will do.

> > It needs to be noted that we can not add checking for xobj member functions to
> > DECL_NONSTATIC_MEMBER_FUNCTION_P as it is used in cp-objcp-common.cc. While it
> > most likely would be fine, it's possible it could have unintended effects. In
> > light of this, we will most likely need to do some refactoring, possibly
> > renaming and replacing it. In contrast, DECL_FUNCTION_MEMBER_P is not used
> > outside of C++ code, so we can add checking for xobj member functions to it
> > without any concerns.
> 
> 
> I think DECL_NONSTATIC_MEMBER_FUNCTION_P should probably be renamed to
> DECL_IOBJ_MEMBER_FUNC_P to parallel the new macro...
> 
> > @@ -3660,6 +3660,7 @@ build_min_non_dep_op_overload (enum tree_code op,
> > 
> > expected_nargs = cp_tree_code_length (op);
> > if (TREE_CODE (TREE_TYPE (overload)) == METHOD_TYPE
> > + || DECL_XOBJ_MEMBER_FUNC_P (overload)
> 
> 
> ...and then the combination should have its own macro, perhaps
> DECL_OBJECT_MEMBER_FUNC_P, spelling out OBJECT to avoid visual confusion
> with either IOBJ/XOBJ.
> 
> Renaming the old macro doesn't need to happen in this patch, but adding
> the new macro should.

Sounds good, I will add it in the next revision.

> > There are a few known issues still present in this patch. Most importantly,
> > the implicit object argument fails to convert when passed to by-value xobj
> > parameters. This occurs both for xobj parameters that match the argument type
> > and xobj parameters that are unrelated to the object type, but have valid
> > conversions available. This behavior can be observed in the
> > explicit-obj-by-value[1-3].C tests. The implicit object argument appears to be
> > simply reinterpreted instead of any conversion applied. This is elaborated on
> > in the test cases.
> 
> 
> Yes, that's because of:
> 
> > @@ -9949,7 +9951,8 @@ build_over_call (struct z_candidate cand, int flags, tsubst_flags_t complain)
> > }
> > }
> > / Bypass access control for 'this' parameter. */
> > - else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
> > + else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE
> > + || DECL_XOBJ_MEMBER_FUNC_P (fn))
> 
> 
> We don't want to take this path for xob fns. Instead I think we need to
> change the existing:
> 
> > gcc_assert (first_arg == NULL_TREE);
> 
> 
> to assert that if first_arg is non-null, we're dealing with an xob fn,
> and then go ahead and do the same conversion as the loop body on first_arg.
> 
> > Despite this, calls where there is no valid conversion
> > available are correctly rejected, which I find surprising. The
> > explicit-obj-by-value4.C testcase demonstrates this odd but correct behavior.
> 
> 
> Yes, because checking for conversions is handled elsewhere.

Yeah, as I noted above I realized that just handling it the same way as
iobj member functions is fundamentally broken. I was staring at it last
night and eventually realized that I could just copy the loop body. I
ended up asserting in the body handling the implicit object argument
for xobj member functions that first_arg != NULL_TREE, which I wasn't
sure of, but it seems to work.

I tried asking in IRC if there are any circumstances where first_arg
would be null for a non-static member function and I didn't get an
answer. The code above seemed to indicate that it could be. It just
looks like old code that is no longer valid and never got removed.
Consequently this function has made it on my list of things to refactor
:^).

It does give me confidence that I made the right call seeing you point
out the same things though.

> > Other than this, lambdas are not yet supported,
> 
> 
> The error I'm seeing with the lambda testcase is "explicit object member
> function cannot have cv-qualifier". To avoid that, in
> cp_parser_lambda_declarator_opt you need to set quals to
> TYPE_UNQUALIFIED around where we do that for mutable lambdas.

Yeah, this ended up being the case as suspected, and it was exactly in
cp_parser_lambda_declarator_opt that it needed to be done. There's an
additional quirk in start_preparsed_function, which I already rambled
about a little in a previous reply on this chain, that suddenly
restored setting up the 'this' pointer. Previously, it was being
skipped because ctype was not being set for xobj member functions.
However, ctype not being set was causing the scope to not be set up
correctly for lambdas. In truth I don't remember exactly how the
problem was presenting but I do know how I fixed it.

```
  tree fntype = TREE_TYPE (decl1);
  if (TREE_CODE (fntype) == METHOD_TYPE)
    ctype = TYPE_METHOD_BASETYPE (fntype);
  else if (DECL_XOBJ_MEMBER_FUNC_P (decl1))
    ctype = DECL_CONTEXT (decl1);
```

In hindsight though, I have to question if this is the correct way of
dealing with it. As I mentioned previously, there's an additional quirk
in start_preparsed_function where it sets up the 'this' param, or at
least, it kind of looks like it? This is where my rambling was about.

```
  /* 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)))
```

My solution was to just not enter that block for xobj member functions,
but I'm realizing now that ctype doesn't seem to be set for static
member functions so setting ctype for xobj member functions might be
incorrect. I'll need to investigate it closer, I recall seeing the
second block above and thinking "it's checking to make sure decl1 is
not a static member function before entering this block, so that means
it must be set." It seems that was incorrect.

At bare minimum, I'll have to look at this again.

> > and there is some outstanding
> > odd behavior where invalid calls to operators are improperly accepted. The
> > test for this utilizes requires expressions to operate though so it's possible
> > that the problems originate from there, but I have a hunch they aren't
> > responsible. See explicit-obj-ops-requires-mem.C and
> > explicit-obj-ops-requires-non-mem.C for those tests.
> 
> 
> I think that's the same issue as by-value1.C above.
> 
> Though you're also missing a couple of semicolons on
> 
> > + RRef&& operator()(this RRef&& self) { return static_cast<RRef&&>(self) }
> > + RRef&& operator[](this RRef&& self) { return static_cast<RRef&&>(self) }

Yeah, those semicolons and it also turns out that both this and the
other failing requires test were just semantically incorrect in a few
places. I have fixed them and they will be in the next version.

I mistakenly was considering passing a rvalue ref to a const rvalue ref
as ill-formed. That was the obvious one. That's what I get for writing
tests at 7am though.

The less obvious were the address-of and comma operators. I was
puzzling over why it was specifically the lvalue ref cases that were
failing for address-of, and eventually I realized that the built-in in
address-of operator is only valid for lvalues. Once I realized that, I
realized that the comma operator's unexpected successes was also the
same thing, it was just using the built-in operators. I solved this by
checking the return type for those two cases specifically.

Finally, for the arrow operator template cases I just had a const in
the wrong place, once that was moved it worked.

Both of the tests pass now, the next version of the patch will have
them enabled. The operator related tests by far have given me the most
problems out of all the tests. I'm super glad to be done with them...

> > @@ -7164,6 +7164,12 @@ cp_build_addr_expr_1 (tree arg, bool strict_lvalue, tsubst_flags_t complain)
> > && !mark_used (t, complain) && !(complain & tf_error))
> > return error_mark_node;
> > 
> > + /* Exception for non-overloaded explicit object member function.
> > + I am pretty sure this is not perfect, I think we aren't
> > + handling some constexpr stuff, but I am leaving it for now. */
> > + if (TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE)
> > + return build_address (t);
> 
> 
> Specifically, you're missing the SET_EXPR_LOCATION at the end of the
> function. Maybe break here instead of returning?

I'll play around with this, now that you mention it I seem to recall
incorrect diagnostics with this so that must be why. This one just took
me a week or two to fix so once I had it I really wanted to move on to
something else. I've got a lot more cleared off so I'll come back to it
today.

> > +// missing test for three-way-compare (I don't know how to write it)
> > +// missing test for ->* (I don't know how to write it)
> 
> 
> Following the same pattern seems to work for me, e.g. (OPERAND) <=> 0
> 
> and (OPERAND) ->* 0.

I will take a crack at it, it's more writing the operator overloads
themselves that gives me brain damage. Guess I'm not done with these
tests after all.

> > +template<typename T = void>
> > +void do_calls()
> 
> 
> You probably want to instantiate the templates in these testcases to
> make sure that works.

Oh jeez, I did forget to in that test, I will fix that... man, I hope
this test isn't suddenly failing once I do that.


> > +// It's very hard to test for incorrect successes without requires, and by extension a non dependent variable
> > +// so for the time being, there are no non dependent tests invalid calls.
> 
> 
> I don't understand the problem, you can have requires-expressions in
> non-dependent context.
> 
> Jason

Yeah that's what I had thought too, but it was giving me errors. I seem
to recall inconsistent behavior with this so maybe it's a new bug? I
just wouldn't be surprised if this was actually the correct behavior
though, it seems consistent with how static_assert's are with non
dependent expressions. I will ask around, do some testing on godbolt,
maybe try to read the standard. Ultimately though I'm not really sure,
I just know I couldn't get it to work without making the operands
dependent.

To be clear, it was giving an error on the expression being tested in
the requires expression. The cases that are supposed to intentionally
fail were giving this error.

Thanks for the input, one more e-mail to respond to then back to work.

Alex