On 11/11/23 05:43, waffl3x wrote:
[combined reply to all three threads]
On 11/9/23 23:24, waffl3x wrote:
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 will check to see what the behavior with this is. This sounds related
to the next question I asked as well.
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?
Yeah I agree that it's easier to store it separately.
-- call.cc:build_over_call
```
else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
{
tree arg = build_this (first_arg != NULL_TREE
? first_arg
: (*args)[arg_index]);
```
The trouble is, the code (shown above) does not assume that this holds
true. It handles the case where the implicit object argument was passed
in with the rest of the arguments. As far as I've observed, it seems
like it's always passed in through the first_arg member of cand, which
is what I was referring to here.
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.
Since it wasn't clear what I was referring to, here is the code that I
wrote (copied from the loop really) handling the case. In case it isn't
obvious, I didn't snip the code in the METHOD_TYPE block, it's just
snipped here as it's not code I've modified. I'm hopeful that the case
you mentioned above is not problematic, but like I said I will be sure
to test it.
-- call.cc:build_over_call
```
else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
{
/* SNIP */
if (first_arg != NULL_TREE)
first_arg = NULL_TREE;
else
++arg_index;
++i;
is_method = 1;
}
else if (DECL_XOBJ_MEMBER_FUNC_P (fn))
{
gcc_assert (cand->first_arg);
gcc_assert (cand->num_convs > 0);
tree type = TREE_VALUE (parm);
tree arg = cand->first_arg;
bool conversion_warning = true;
conv = convs[0];
/* Set user_conv_p on the argument conversions, so rvalue/base handling
knows not to allow any more UDCs. This needs to happen after we
process cand->warnings. */
if (flags & LOOKUP_NO_CONVERSION)
conv->user_conv_p = true;
tsubst_flags_t arg_complain = complain;
if (!conversion_warning)
arg_complain &= ~tf_warning;
if (arg_complain & tf_warning)
maybe_warn_pessimizing_move (arg, type, /*return_p*/false);
val = convert_like_with_context (conv, arg, fn, 0,
arg_complain);
val = convert_for_arg_passing (type, val, arg_complain);
if (val == error_mark_node)
return error_mark_node;
else
argarray[j++] = val;
/* Advance parameter chain, don't advance arg index */
parm = TREE_CHAIN (parm);
// ++arg_index;
++i;
is_method = 1;
first_arg = NULL_TREE;
}
```
Anyway, I hope we can assume that first_argument is populated for all
non-static member function calls, and if we can't perhaps that is
something we can change. The assumptions we can make are unclear with
how the code handling METHOD_TYPE is right now. If we make it a
guarantee that first_argument is populated for non-static member
function calls that code can be vastly simplified.
Somewhat, but the current METHOD_TYPE code works fine, so let's not mess
with it. There might be corner cases that end up with the object
argument in the args vec.
[other reply]
I already showed my code here but I didn't think to include the
previous conditional block to demonstrate how the first_arg variable
gets handled in it. Although, maybe you understood they set it to
NULL_TREE by the end of the block and I just poorly communicated that I
would be doing the same.
I meant that I expect the
gcc_assert (cand->first_arg);
to fail on my example.
Perhaps you think it better to handle the implicit object argument
within the loop, but given that it is stored in first_arg I don't think
that would be correct. Granted, as I previously said, maybe there are
some situations where it isn't that I haven't encountered yet. The
other thing is there is some handling in the loop that isn't relevant
to the implicit object argument. Well, I would also just argue this
function needs a fair degree of refactoring, but I've already talked
about that.
Factoring out the loop body (into a lambda?) rather than copying it
would make sense to me.
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'll put it on the todo list for after this patch is done. I'll note
that it seemed like not doing push_nested_class seemed to work just
fine for xobj member functions, and only caused problems once I was
working on lambdas.
Hmm, we want push_nested_class for any member function so that name
lookup within the function finds class members.
For this patch, I think I will change the condition
for setting up 'this' instead. On the other hand, are we sure that
lambdas don't assume 'this' was setup when accessing their captures? I
will investigate this briefly but now that I've thought of that, maybe
I won't mess around with this after all. On the other other hand, I'm
pretty sure captures still work for lambdas even though that block
isn't being handled now... or maybe I haven't tested them.
Captures don't rely on being able to name the object parameter, they use
it directly without name lookup. And indeed 'this' doesn't name the
object parameter for the lambda op(), finish_this_expr looks past it.
Actually, I think it doesn't matter if lambdas do use that or not for
their captures, because handling lambdas with an xobj parameter
correctly will still involve changing how lambdas access their
captures. PR102609 has a reply providing a test case when calling a
lambdas call operator where the first argument is a type unrelated to
the lambda's closure type. This might actually be relevant because I
can think of a case where one could conditionally (if constexpr) access
captures based on the type of the xobj parameter. THEN AGAIN, I don't
think it's possible to pass an unrelated implicit object parameter to a
lambda's call operator. (Not without explicitly calling it by function
pointer of course.) There are types that are derived from the lambda,
but those are still related so I assume that captures should be able to
be accessed in such a case.
Man, that raises even more questions for myself. If we are supposed to
implicitly use the explicit object parameter for accessing captures...
([PR102609] Gašper Ažman from comment #17)
When a lambda has captures, the explicit object parameter is used to get at
them *silently*, if they are related, otherwise the program is ill-formed:
...then what happens when the type of the explicit object parameter is
a class derived from the lambda, and the derived class has a member
variable with the same name as a capture we are trying to access?
That ought to work fine; capture fields don't have the name of the
variables they capture, they are only accessible through the capture
proxy (from build_capture_proxy), which in turn refers directly to the
member by its _DECL, not by name lookup.
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.
I believe so, consider the following:
struct S {
void f(this S) {}
void f(this S const&) {}
void f(this S&&) {}
};
I'm not 100% what the rules on this are, if I were to hazard a guess I
bet that the reference overloads are taken because they have one less
conversion?
Initializing a by-value parameter isn't an extra conversion, it's an
identity conversion, so ambiguous with the reference overloads, just as with
struct A { };
void f (A);
void f (const A&);
void f (A&&);
int main()
{
A a;
f (a); // ambiguous
f (A()); // ambiguous
}
To be clear, I recognize that you could still select the by-value
candidate by assigning it to a pointer to function type. I just don't
really think this is a useful or typical case, while accidentally
preventing a by-value xobj member function from being callable seems
like something that could easily happen, especially if we had the
following.
struct S {
template<typename Self>
void f(this Self&&) {}
/* Our class is small enough, optimize const calls here. */
void f(this S) {}
}
I could see this being a typical enough refactor that I would want a
warning to alert me that my function that I'm writing for optimization
purposes won't ever be called in the cases I intend it to be called in.
Granted, I fully admit that I don't fully understand the semantics with
overload resolution here so my concerns might be entirely unfounded.
This is especially true in the second case since I'm not sure if the
deduced case actually will eat up all the calls of the member function
or not.
Rather, in the deduced case the by-value overload always wins because
overload resolution prefers a non-template if the conversions are
equally good.
struct A { };
void f (A);
template <class T> void f (T&&);
int main()
{
A a;
f (a); // calls non-template
f (A()); // calls non-template
}
I'll try to get a new version of the patch out ASAP, I'm not sure I'll
include everything we talked about right away but I'll stuff in as much
as I can. I'll focus on the missing semantics before the more
complicated diagnostics changes. I think detection of redeclarations,
the correct overriding behavior, and the additional warnings I have
been discussing, will come later.
Makes sense.
I think I'm also going to leave investigation of requires expressions
in non-dependent contexts that I had mentioned for another time as
well. The tests related to those have been fixed and work as expected
right now, and I think diving in and trying to figure out whether I can
do those tests with non-dependent operands would be a bad use of my
time.
Agreed.
Of course I will make sure all the small changes you requested make it
in, that might be the first thing I work on.
Also note that revisions can slip into Stage 3, though preferably not
too far.
Jason
