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

[Jason Merrill <jason@redhat.com>]

[combined reply to all three threads]

On 11/9/23 23:24, waffl3x wrote:
> 
>>> I'm unfortunately going down a rabbit hole again.
>>>
>>> --function.h:608
>>> `/* If pointers to member functions use the least significant bit to indicate whether a function is virtual, ensure a pointer to this function will have that bit clear. */ #define MINIMUM_METHOD_BOUNDARY \\ ((TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn) \\ ? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)`
>>
>>
>> So yes, it was for PMFs using the low bit of the pointer to indicate a
>> virtual member function. Since an xob memfn can't be virtual, it's
>> correct for them to have the same alignment as a static memfn.
> 
> Is it worth considering whether we want to support virtual xobj member
> functions in the future? If that were the case would it be better if we
> aligned things a little differently here? Or might it be better if we
> wanted to support it as an extension to just effectively translate the
> declaration back to one that is a METHOD_TYPE? I imagine this would be
> the best solution for non-standard support of the syntax. We would
> simply have to forbid by-value and conversion semantics and on the
> user's side they would get consistent syntax.
> 
> However, this flies in the face of the defective/contradictory spec for
> virtual function overrides. So I'm not really sure whether we would
> want to do this. I just want to raise the question before we lock in
> the alignment, if pushing the patch locks it in that is, I'm not really
> sure if it needs to be stable or not.

It doesn't need to be stable; we can increase the alignment of decls as 
needed in new code without breaking older code.

>>> All tests seemed to pass when applied to GCC14, but the results did
>>> something funny where it said tests disappeared and new tests appeared
>>> and passed. The ones that disappeared and the new ones that appeared
>>> looked like they were identical so I'm not worrying about it. Just
>>> mentioning it in case this is something I do need to look into.
>>
>> That doesn't sound like a problem, but I'm curious about the specific
>> output you're seeing.
> 
> I've attached a few test result comparisons so you can take a look.

Looks like you're comparing results from different build directories and 
the libitm test wrongly includes the build directory in the test "name". 
  So yeah, just noise.

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

Agreed.

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

That sounds like it might cause trouble with

struct A {
    void f(this A);
};

int main()
{
   (&A::f) (A());
}

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

Right, first_arg is only actually used for the implicit object argument, 
it's just easier to store it separately from the arguments in ().  I'm 
not sure which code you mean is no longer valid?

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

Yeah, that could use some refactoring.  IMO ctype should be set iff we 
want to push_nested_class, and the code for setting up 'this' should 
check for METHOD_TYPE instead of referring to ctype.

>>> 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.
> 
> Ah I guess I should have read all your responses before sending any
> back instead of going one by one. I ended up writing about this again I
> think.
> 
> struct B {
>     virtual void f() const {}
> };
> 
> struct S0 : B {
>     void f() {}
> };
> 
> struct S1 : B {
>     void f(this S1&) {}
> };
> 
> I had a bit of a debate with a peer, I initially disagreed with the
> standard resolution because it disallows S1's declaration of f, while
> S0's is an overload. I won't bore you with all the details of going
> back and forth about the proposed wording, my feelings are mostly that
> being able to overload something with an iobj member function but not
> being able to with an xobj member function was inconsistent. He argued
> that keeping restrictions high at first and lowering them later is
> better, and overloading virtual functions is already not something you
> should really ever do, so he was in favor of the proposed wording.

Right.  As the author of this proposal said in discussion, "We want very 
liberal overriding for explicit object member functions so that it'll 
blow up."

> In light of our debate, my stance is that we should implement things as
> per the proposed wording. 
> 
> struct B {
>   virtual void foo() const&;
> };
> 
> struct D : B {
>   void foo(this int);
> };
> 
> This would be ill-formed now according to the change in wording. My
> laymans interpretation of the semantics are that, the parameter list is
> the same when the xobj parameter is ignore, so it overrides. And since
> it overrides, and xobj member functions are not allowed to override, it
> is an error.

Yes.

> To be honest, I still don't understand where the wording accounts for
> the qualifiers of B::f, but my friend assured me that it is.

The qualifiers of B::f are part of the object parameter which we're 
ignoring because the derived function is xobj.

> Somewhat related is some warnings I wanted to implement for impossible
> to call by-value xobj member functions. Ones that involve an unrelated
> type get dicey because people could in theory have legitimate use cases
> for that, P0847R7 includes an example of this combining recursive
> lambdas and the overload pattern to create a visitor. However, I think
> it would be reasonable to warn when a by-value xobj member function can
> not be called due to the presence of overloads that take references.
> Off the top of my head I don't recall how many overloads it would take
> to prevent a by-value version from being called, nor have I looked into
> how to implement this.

Hmm, is it possible to make it un-callable?  A function taking A and a 
function taking A&& are ambiguous when called with an rvalue argument, 
the by-value overload isn't hidden.  Similarly, A and A& are ambiguous 
when called with a cv-unqualified lvalue.

> But I do know that redeclarations of xobj member
> functions as iobj member functions (and vice-versa) are not properly
> identified when ref qualifiers are omitted from the corresponding (is
> this the correct term here?) iobj member function.

That would be the code in add_method discussed later in that message.

>>> + /* If */
>>
>> I think this comment doesn't add much. :)
> 
> I wish I remembered what I even meant to put here, oh well, must not
> have been all that important. Oh wait, I remember it now, I was going
> to note that the next element in the chain of declarators being null
> seems to indicate that the declaration is a function type. I will
> probably put that comment in again, it's on the line of commenting
> exactly what the code does but it was cryptic to figure this out, I'm
> not sure if I should lean towards including the comment here or not.

I always lean towards including comments. :)

>>> + /* 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)?
> 
> I've been reluctant to make any changes to parameters, but if you're
> suggesting it I will consider it. Just replacing it with '(&name)'
> seems really mediocre to me though. Yeah, sure, it's always going to be
> syntactically valid but I worry that it wont be semantically correct
> often enough.
> 
> Yeah, considering that I think I will leave it for now until I have
> time to come up with a robust solution. I might still pass in the
> location of the this token though and use error_at instead of error.
> I'll see how I feel once I finish higher priority stuff.

Makes sense.

> To be clear, I should be replacing { dg-options "-Wno-error=pedantic" }
> with { dg-options "" } and you would prefer I just remove { dg-options
> "-pedantic-errors" } yeah?

Yes.

Jason