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.

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

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

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

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

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

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

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

> +template<typename T = void>
> +void do_calls()

You probably want to instantiate the templates in these testcases to 
make sure that works.

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