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

[waffl3x <waffl3x@protonmail.com>]

I've been under the weather so I took a few days break, I honestly was
also very reluctant to pick it back up. The current problem is what I
like to call "not friendly", but I am back at it now.

> > I don't understand what this means exactly, under what circumstances
> > would &f find the member function. Oh, I guess while in the body of
> > it's class, I hadn't considered that. Is that what you're referring to?
>
>
> Right:
>
> struct A {
> void g(this A&);
> A() {
> &A::g; // ok
> &g; // same error as for an implicit object member function
> }
> };

I fully get this now, I threw together a test for it so this case
doesn't get forgotten about. Unfortunately though, I am concerned that
the approach I was going to take to fix the crash would have the
incorrect behavior for this.

Here is what I added to cp_build_addr_expr_1 with context included.
```
    case OFFSET_REF:
    offset_ref:
      /* Turn a reference to a non-static data member into a
	 pointer-to-member.  */
      {
	tree type;
	tree t;

	gcc_assert (PTRMEM_OK_P (arg));

	t = TREE_OPERAND (arg, 1);
	if (TYPE_REF_P (TREE_TYPE (t)))
	  {
	    if (complain & tf_error)
	      error_at (loc,
			"cannot create pointer to reference member %qD", t);
	    return error_mark_node;
	  }
        /* -- Waffl3x additions start -- */
	/* Exception for non-overloaded explicit object member function.  */
	if (TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE)
	  return build1 (ADDR_EXPR, unknown_type_node, arg);
        /* -- Waffl3x additions end -- */

	/* Forming a pointer-to-member is a use of non-pure-virtual fns.  */
	if (TREE_CODE (t) == FUNCTION_DECL
	    && !DECL_PURE_VIRTUAL_P (t)
	    && !mark_used (t, complain) && !(complain & tf_error))
	  return error_mark_node;
```

I had hoped this naive solution would work just fine, but unfortunately
the following code fails to compile with an error.

```
struct S {
    void f(this S&) {}
};
int main() {
    void (*a)(S&) = &S::f;
}
```
normal_static.C: In function ‘int main()’:
normal_static.C:13:25: error: cannot convert ‘S::f’ from type ‘void(S&)’ to type ‘void (*)(S&)’
   13 |     void (*a)(S&) = &S::f;
      |                         ^

So clearly it isn't going to be that easy. I went up and down looking
at how the single static case is handled, and tried to read the code in
build_ptrmem_type and build_ptrmemfunc_type but I had a hard time
figuring it out.

The good news is, this problem was difficult enough that it made me
pick a proper diff tool with regex support instead of using a silly web
browser tool and pasting things into it. Or worse, pasting them into a
tool and doing replacement and then pasting them into the silly web
browser tool. I have been forced to improve my workflow thanks to this
head scratcher. So it's not all for naught.

Back on topic, it's not really the optimization returning a baselink
that is causing the original crash. It's just the assert in
build_ptrmem_type failing when a FUNCTION_TYPE is reaching it. The
optimization did throw me for a loop when I was comparing how my
previous version (that incorrectly set the lang_decl_fn ::
static_function flag) was handling things. Looking back, I think I
explained myself and the methodology I was using to investigate really
poorly, I apologize for the confusion I might have caused :).

To state it plainly, it seems to me that the arg parameter being passed
into cp_build_addr_expr_1 for explicit object member functions is
(mostly) pretty much correct and what we would want.

So the whole thing with the baselink optimization was really just a red
herring that I was following. Now that I have a better understanding of
what's happening leading up to and in cp_build_addr_expr_1 I don't
think it's relevant at all for this problem. With that said, I am
questioning again if the optimization that returns a baselink node is
the right way to do things. So this is something I'm going to put down
into my little notes text file to investigate at a later time, and
forget about it for the moment as it shouldn't be causing any friction
for us here.

Anyway, as I eluded to above, if I can't figure out the right way to
solve this problem in a decent amount of time I think I'm going to
leave it for now. I'll come back to it once other higher priority
things are fixed or finished. And hopefully someone more familiar with
this area of the code will have a better idea of what we need to do to
handle this case in a non-intrusive manner.

That wraps up my current status on this specifically. But while
investigating it I uncovered a few things that I feel are important to
discuss/note.

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.


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.

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?

4. I have no comment here, but it does concern me since I don't
understand it at all.

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?

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. The baselink binfo field has the private flag set for xobj
member functions, iobj member functions does not.

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.

Alex