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

[waffl3x <waffl3x@protonmail.com>]

On Wednesday, November 29th, 2023 at 10:00 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/27/23 00:35, waffl3x wrote:
> 
> > I think this is cleaned up enough to be presentable. Bootstrapped but
> > not tested but I don't think I changed anything substantial. I am
> > running tests right now and will report if anything fails. I have not
> > fixed the problem in tsubst_lambda_expr that we talked about, that will
> > be first on my list tomorrow. While writing/cleaning up tests I had to
> > investigate some things, one of which is calling an overloaded
> > function, where one of the candidates are introduced by a using
> > declaration, is considered ambiguous. I haven't narrowed down the case
> > for this yet so I don't know if it's related to xobj member
> > functions/lambda with xobj parameters or not. I had to pull a few tests
> > because of it though.
> > 
> > I did not get as much done as I would have hoped today. This really
> > just serves as a small progress update. Once again, todo is the issue
> > you raised in tsubst_lambda_expr, and fixing handling of captures when
> > a const xobj parameter is deduced in a lamdba call operator.
> 
> > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> > +#define DECL_XOBJ_MEMBER_FUNC_P(NODE) \
> > +#define DECL_OBJECT_MEMBER_FUNC_P(NODE) \
> 
> 
> Let's use the full word FUNCTION in these macros for consistency with
> DECL_STATIC_FUNCTION_P.

Okay.

> > @@ -6544,7 +6544,7 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
> > tree fn_first_arg = NULL_TREE;
> > const vec<tree, va_gc> *fn_args = args;
> > 
> > - if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn))
> > + if (DECL_OBJECT_MEMBER_FUNC_P (fn))
> > {
> > /* Figure out where the object arg comes from. If this
> > function is a non-static member and we didn't get an
> 
> 
> Hmm, that this function explicitly pulls out the object argument into
> first_arg strengthens your earlier argument that we shouldn't bother
> trying to handle null first_arg. But let's not mess with that in this
> patch.

Maybe I'll take some time to look into it when this patch is done, I
came across another block that seems to guarantee that first_arg gets
passed in a bit ago.

> > + val = handle_arg(TREE_VALUE (parm),
> 
> 
> Missing space.

Is there a script I can use for this so I'm not wasting your time on
little typos like this one?

> > /* We know an iobj parameter must be a reference. If our xobj
> > parameter is a pointer, we know this is not a redeclaration.
> > This also catches array parameters, those are pointers too. */
> > if (TYPE_PTR_P (xobj_param))
> > continue;
> 
> 
> Handling pointers specifically here seems unnecessary, they should be
> rare and will be handled by the next check for unrelated type.

Ah, it took me a second but I see it now, yeah I think I'll make this
change, with a comment that notes that it also handles the pointer case.

> > dealing with a by-value xobj parameter we can procede following
> 
> 
> "proceed"
> 
> > /* An operator function must either be a non-static member function
> > or have at least one parameter of a class, a reference to a class,
> > an enumeration, or a reference to an enumeration. 13.4.0.6 /
> > - if (! methodp || DECL_STATIC_FUNCTION_P (decl))
> > + / This can just be DECL_STATIC_FUNCTION_P (decl) I think? */
> > + if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
> > + || DECL_STATIC_FUNCTION_P (decl))
> 
> 
> No, it also needs to be true for non-members; rather, I think it can
> just be if (!DECL_OBJECT_FUNCTION_P (decl))

Yeah that seems to make sense, I'll try that.

> > + if (xobj_param)
> > + {
> > + quals = TYPE_UNQUALIFIED;
> > + if (TYPE_REF_P (xobj_param)
> > + && !(cp_type_quals (TREE_TYPE (xobj_param)) & TYPE_QUAL_CONST))
> > + LAMBDA_EXPR_MUTABLE_P (lambda_expr) = 1;
> > + }
> 
> 
> I don't think we want to mess with MUTABLE_P here. But then
> capture_decltype needs to be fixed to refer to the type of the object
> parameter rather than MUTABLE_P.
> 
> Actually, I think we can do away with the MUTABLE_P flag entirely. I'm
> going to push a patch to do that.

I've been working on code that concerns it today and yesterday and I
was beginning to think the same thing. My current version doesn't set
it at all. I'm happy to see it removed.

When I looked into whether I should or should not be setting it I found
a new bug with decltype((capture)) in lambdas with default capture. I
am not going to try to fix the value capture default case but I was
able to fix the ref capture default case. Basically decltype((x)) is
not dependent right now as far as I can tell. Both MSVC and clang have
the same bug so I'm not worried about it tbh.

> > - if (!LAMBDA_EXPR_STATIC_P (lambda_expr))
> > + if (!LAMBDA_EXPR_STATIC_P (lambda_expr)
> > + && !DECL_XOBJ_MEMBER_FUNC_P (fco))
> 
> 
> This could be if (DECL_IOBJ_MEMBER_FUNCTION_P (fco))
> 
> > - if (closure && !DECL_STATIC_FUNCTION_P (t))
> > + if (closure && DECL_IOBJ_MEMBER_FUNC_P (t) && !DECL_STATIC_FUNCTION_P (t))
> 
> 
> This shouldn't need to still check DECL_STATIC_FUNCTION_P.

Yeah... not sure why I left that.

> > + /* We don't touch a lambda's func when it's just trying to create the
> > + closure type. */
> 
> 
> We need to check it somewhere, currently this crashes:
> 
> template <class T> void f()
> 
> {
> int i;
> [=](this T&& self){ return i; }(); // error, unrelated
> }
> int main() { f<int>(); }

Ah, yep you're right, how silly of me to forget that case. I'll make
sure to add it as a test case as well.

> > @@ -3691,18 +3691,7 @@ build_min_non_dep_op_overload (enum tree_code op,
> > releasing_vec args;
> > va_start (p, overload);
> > 
> > - if (TREE_CODE (TREE_TYPE (overload)) == FUNCTION_TYPE)
> > - {
> > - fn = overload;
> > - if (op == ARRAY_REF)
> > - obj = va_arg (p, tree);
> > - for (int i = 0; i < nargs; i++)
> > - {
> > - tree arg = va_arg (p, tree);
> > - vec_safe_push (args, arg);
> > - }
> > - }
> 
> 
> Maybe change the test to !DECL_OBJECT_MEMBER_FUNC_P to avoid reordering
> the cases?

I thought you had already given the thumbs up to this, but I'll take
another look at it and see what feels good.

> > @@ -15402,6 +15450,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain,
> > gcc_checking_assert (TYPE_MAIN_VARIANT (TREE_TYPE (ve))
> > == TYPE_MAIN_VARIANT (type));
> > SET_DECL_VALUE_EXPR (r, ve);
> > + if (is_capture_proxy (t))
> > + type = TREE_TYPE (ve);
> 
> 
> That should have close to the same effect as the lambda_proxy_type
> adjustment I was talking about, since that function basically returns
> the TREE_TYPE of the COMPONENT_REF. But the underlying problem is that
> finish_non_static_data_member assumes that 'object' is '*this', for
> which you can trust the cv-quals; for auto&&, you can't.
> capture_decltype has the same problem. I'm attaching a patch to address
> this in both places.
> 
> Jason

Ah perfect, you also noticed the decltype problem. I don't think I
could have fixed this one on my own so thanks for addressing it.

I started replacing old uses of DECL_NONSTATIC_MEMBER_FUNCTION_P in the
cases where I could tell for sure what the correct thing to do is and I
ended up finding a few bugs.

template<typename T>
concept is_int = __is_same(T, int);

struct S {
  static int f(is_int auto a) { return 5; };
  int f(this S&, auto a) { return 10; };
};

int main()
{
  S s{};
  return s.f(0);
}

This program returns 10 instead of 5.

https://godbolt.org/z/jMfbbaczr
Actually, I found that this bug exists in MSVC as well. Here it is in
godbolt with some extra things to make it more obvious where the bug
is. I've already found the cause of it, cand_parms_match doesn't ignore
the xobj parameter. It's an easy fix thankfully. I feel like these can
be unified because I'm pretty sure I've seen this in 2 or 3 places, but
I'll concern myself with trying to refactor it later.

I still have a case that I haven't fully narrowed down yet that
considers a call ambiguous when one of the candidates were introduce by
a using declaration. That's next on my list I believe.

I also found that xobj conversion operators were not working. I have
already fixed it but I need to write more robust tests for it to make
sure all the cases work. There's also a case where an xobj conversion
operator has an unrelated type for it's parameter and I haven't been
able to decide if it's ill-formed or not. I believe it would break
conversion sequence rules for a conversion operator to be selected.
Once I finish everything else I'll get a good robust test case for it.

This ended up longer than I thought. To recap, I have the bug with
constraints which should be fixed but needs tests, I have the wrongly
ambiguous overload bug that I need to narrow down and find, and I have
xobj conversion operators to write tests for. So after I address what
you said that's whats on my list. There's also integrating your patch
which I'm not totally sure how I'm supposed to do but I think I have a
good idea of it so I should be fine.

I feel like I'm forgetting something, but that often happens when I
stack up like 4 different things on my mind at once so it's probably
nothing. I'll get back to work. At this rate it almost feels like we
will be able to get this finished by the end of the week.

Alex