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

[Jason Merrill <jason@redhat.com>]

On 10/28/23 00:07, waffl3x wrote:
> I wanted to change DECL_NONSTATIC_MEMBER_FUNCTION_P to include explicit
> object member functions, but it had some problems when I made the
> modification. I also noticed that it's used in cp-objcp-common.cc so
> would making changes to it be a bad idea?
> 
> -- cp-tree.h
> ```
> /* Nonzero for FUNCTION_DECL means that this decl is a non-static
>     member function.  */
> #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
>    (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> ```
> I didn't want to investigate the problems as I was knee deep in
> investigating the addressof bug. So I instead modified
> DECL_FUNCTION_MEMBER_P to include explicit object member functions and
> moved on.
> 
> -- cp-tree.h
> ```
> /* Nonzero for FUNCTION_DECL means that this decl is a member function
>     (static or non-static).  */
> #define DECL_FUNCTION_MEMBER_P(NODE) \
>    (DECL_NONSTATIC_MEMBER_FUNCTION_P (NODE) || DECL_STATIC_FUNCTION_P (NODE) \
>    || DECL_IS_XOBJ_MEMBER_FUNC (NODE))
> ```
> I am mostly just mentioning this here in case it becomes more relevant
> later. Looking at how much DECL_NONSTATIC_MEMBER_FUNCTION_P is used
> throughout the code I now suspect that adding explicit object member
> functions to it might cause xobj member functions to be treated as
> regular member functions when they should not be.
> 
> If this change were to stick it would cause a discrepancy in the
> behavior of DECL_NONSTATIC_MEMBER_FUNCTION_P and it's name. If we were
> to do this, I think it's important we document the discrepancy and why
> it exists, and in the future, it should possibly be refactored. One
> option would be to simply rename it to DECL_IOBJ_MEMBER_FUNCTION_P.
> After all, I suspect that it's unlikely that the current macro
> (DECL_NONSTATIC_MEMBER_FUNCTION_P) is being used in places that concern
> explicit object member functions. So just adding explicit object member
> functions to it will most likely just result in headaches.
> 
> It seems to me that would be the best solution, so when and if it comes
> up again, I think that route should be considered.

Agreed, it sounds good to rename the current macro and then add a new 
macro that includes both implicit and explicit, assuming that's a useful 
category.

> Secondly, there are some differences in the nodes describing an
> explicit object member function from those describing static member
> functions and implicit object member functions that I am not sure
> should be present.
> 
> I did my best to summarize the differences, if you want the logs of
> tree_debug that I derived them from I can provide them. Most of my
> understanding of the layout of the nodes is from reading print-tree.cc
> and looking at debug_tree outputs, so it's possible I made a mistake.
> 
> I am opting to use the names of members as they are output by
> debug_tree, I recognize this is not always the actual name of the
> member in the actual tree_node structures.
> 
> Additionally, some of the differences listed are to be expected and are
> most likely the correct values for each node. However, I wanted to be
> exhaustive when listing them just in case I am mistaken in my opinion
> on whether the differences should or should not occur.
> 
> The following declarations were used as input to the compiler.
> iobj decl:
> struct S { void f() {} };
> xobj decl:
> struct S { void f(this S&) {} };
> static decl:
> struct S { static void f(S&) {} };
> 
> These differences can be observed in the return values of
> grokdeclarator for each declaration.
> 
> 1. function_decl::type::tree_code
> iobj: method_type
> xobj: function_type
> stat: function_type
> 2. function_decl::type::method basetype
> iobj: <record_type 0x7ffff7194c78 S>
> xobj: NULL/no output
> stat: NULL/no output
> 3. function_decl::type::arg-types::tree_list[0]::value
> iobj: <pointer_type>
> xobj: <reference_type>
> stat: <reference_type>
> 4. function_decl::decl_6
> iobj: false/no output
> xobj: false/no output
> stat: true
> 5. function_decl::align
> iobj: 16
> xobj: 8
> stat: 8
> 6. function_decl::result::uid
> iobj: D.2513
> xobj: D.2513
> stat: D.2512
> 7. function_decl::full-name
> iobj: "void S::f()"
> xobj: "void S::f(this S&)"
> 
> Differences 1, 3, and 7 seem obviously correct to me for all 3
> declarations, 6 is a little bizarre to me, but since it's just a UID
> it's merely an oddity, I doubt it is concerning.

Agreed.

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

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

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

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

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

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

Jason