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

[waffl3x <waffl3x@protonmail.com>]

I woke up today and figured it out in about 30 minutes, I don't know if
this solution would be okay but I am running away from
cp_build_addr_expr_1 for now. I think this is another place I will have
the urge to refactor in the future, but if I keep looking at it right
now I am just going to waste all my energy before I finish everything
else.

This line
```
  else if (BASELINK_P (TREE_OPERAND (arg, 1)))
```
near the bottom of the function is almost certainly dead. The switch
case (near the top) for baselink reassigns arg.
```
case BASELINK:
  arg = BASELINK_FUNCTIONS (arg);
```
And I'm pretty sure a baselink's functions can't be a baselink, so that
case near the bottom is almost certainly dead. I've put it in my todo
so I will look at it in the future.

Anyway, here is my new solution, it seems to work, but as indicated in
the comment (for myself) I'm not sure this handles constexpr things
right as near the bottom of this function there is some constexpr
handling. I know I said it took me 30 minutes but I realized I had some
holes I needed to check as I was writing this email, so it was more
like 2 hours.
```
        /* 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;

        /* Exception for non-overloaded explicit object member function.  */
        /* I am pretty sure this is still not perfect, I think we aren't
           handling some constexpr stuff, but I am leaving it for now. */
        if (TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE)
          return build_address (t);

        type = build_ptrmem_type (context_for_name_lookup (t),
                                  TREE_TYPE (t));
        t = make_ptrmem_cst (type, t);
        return t;
      }
```
I'm sharing it because I am still uncertain on whether this is the
ideal solution, this function is kind of a mess in general (sorry) so
it's hard to tell.

There's still a few things in my previous email that I would like
looked at, primarily the differences I observed in returns from
grokdeclarator, but I am glad to finally be moving forward on to other
things. Hopefully I can put together the patches with this one fixed.
(Of course I still have to fix the other bug I found as well.)

I had thought I might try to get lambda support and by value xobj
params working in this patch, but I think that will still be something
I work on once I am finished the initial patch. I still want to get
both of those into GCC14 though so I probably need to speed things up.

Alex

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