[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
