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

[waffl3x <waffl3x@protonmail.com>]

> > That leaves 2, 4, and 5.
> > 
> > 2. I am pretty sure xobj functions should have the struct they are a
> > part of recorded as the method basetype member. I have already checked
> > that function_type and method_type are the same node type under the
> > hood and it does appear to be, so it should be trivial to set it.
> > However I do have to wonder why static member functions don't set it,
> > it seems to be that it would be convenient to use the same field. Can
> > you provide any insight into that?
> 
> 
> method basetype is only for METHOD_TYPE; if you want the containing type
> of the function, that's DECL_CONTEXT.

-- gcc/tree.h:530
#define FUNC_OR_METHOD_CHECK(T)	TREE_CHECK2 (T, FUNCTION_TYPE, METHOD_TYPE)
-- gcc/tree.h:2518
#define TYPE_METHOD_BASETYPE(NODE)			\
  (FUNC_OR_METHOD_CHECK (NODE)->type_non_common.maxval)

The code doesn't seem to reflect that, perhaps since the underlying
node type is the same (as far as I can tell, they both inherit from
tree_type_non_common) it wasn't believed to be necessary.

Upon looking at DECL_CONTEXT though I see it seems you were thinking of
FUNCTION_DECL. I haven't observed DECL_CONTEXT being set for
FUNCTION_DECL nodes though, perhaps it should be? Although it's more
likely that it is being set and I just haven't noticed, but if that's
the case I'll have to make a note to make sure it is being set for xobj
member functions.

I was going to say that this seems like a redundancy, but I realized
that the type of a member function pointer is tied to the struct, so it
actually ends up relevant for METHOD_TYPE nodes. I would hazard a guess
that when forming member function pointers the FUNCTION_DECL node was
not as easily accessed. If I remember correctly that is not the case
right now so it might be worthwhile to refactor away from
TYPE_METHOD_BASETYPE and replace uses of it with DECL_CONTEXT.

I'm getting ahead of myself though, I'll stop here and avoid going on
too much of a refactoring tangent. I do want this patch to make it into
GCC14 after all.

> > 4. I have no comment here, but it does concern me since I don't
> > understand it at all.
> 
> 
> In the list near the top of cp-tree.h, DECL_LANG_FLAG_6 for a
> FUNCTION_DECL is documented to be DECL_THIS_STATIC, which should only be
> set on the static member.

Right, I'll try to remember to check this area in the future, but yeah
that tracks because I did remove that flag. Removing that flag just so
happened to be the start of this saga of bug fixes but alas, it had to
be done.

> > 5. I am pretty sure this is fine for now, but if xobj member functions
> > ever were to support virtual/override capabilities, then it would be a
> > problem. Is my understanding correct, or is there some other reason
> > that iobj member functions have a different value here?
> 
> 
> It is surprising that an iobj memfn would have a different DECL_ALIGN,
> but it shouldn't be a problem; the vtables only rely on alignment being
> at least 2.

I'll put a note for myself to look into it in the future, it's an
oddity at minimum and oddities interest me :^).

> > There are also some differences in the arg param in
> > cp_build_addr_expr_1 that concerns me, but most of them are reflected
> > in the differences I have already noted. I had wanted to include these
> > differences as well but I have been spending too much time staring at
> > it, it's no longer productive. In short, the indirect_ref node for xobj
> > member functions has reference_to_this set, while iobj member functions
> > do not.
> 
> 
> That's a result of difference 3.

Okay, makes sense, I'm mildly concerned about any possible side effects
this might have since we have a function_type node suddenly going
through execution paths that only method_type went through before. The
whole "reference_to_this" "pointer_to_this" thing is a little confusing
because I'm pretty sure that doesn't refer to the actual `this` object
argument or parameter since I've seen it all over the place. Hopefully
it's benign.

> > The baselink binfo field has the private flag set for xobj
> > member functions, iobj member functions does not.
> 
> 
> TREE_PRIVATE on a binfo is part of BINFO_ACCESS, which isn't a fixed
> value, but gets updated during member search. Perhaps the differences
> in consideration of conversion to a base led to it being set or cleared
> differently? I wouldn't worry too much about it unless you see
> differences in access control.

Unfortunately I don't have any tests for private/public access yet,
it's one of the area's I've neglected. Unfortunately I probably won't
put too much effort into writing TOO many more right now as it takes up
a lot of my time. I still have to clean up the ones I currently have
and I have a few I wanted to write that are not yet written.

> > I've spent too much time on this write up, so I am calling it here, it
> > wasn't all a waste of time because half of what I was doing here are
> > things I was going to need to do anyway at least. I still feel like I
> > spent too much time on it. Hopefully it's of some value for me/others
> > later.
> 
> 
> I hope my responses are helpful as well.

Very much so, thank you!

An update on the regression testing, I had one test fail comparing
against commit a4d2b108cf234e7893322a32a7956ca24e283b05 (GCC13) and I'm
not sure if I need to be concerned about it.
libgomp.c++/../libgomp.c-c++-common/for-16.c execution test

I'm going to be starting tests for my patch against trunk now, once
that is finished I should be ready to format a patch for review.

That's all for now, thanks again.

Alex