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

[waffl3x <waffl3x@protonmail.com>]

I'm unfortunately going down a rabbit hole again.

--function.h:608
```
/* If pointers to member functions use the least significant bit to
   indicate whether a function is virtual, ensure a pointer
   to this function will have that bit clear.  */
#define MINIMUM_METHOD_BOUNDARY \
  ((TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn)	     \
   ? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)
```
--decl.cc:10400 grokfndecl
```
  if (TREE_CODE (type) == METHOD_TYPE)
    {
      tree parm = build_this_parm (decl, type, quals);
      DECL_CHAIN (parm) = parms;
      parms = parm;

      /* Allocate space to hold the vptr bit if needed.  */
      SET_DECL_ALIGN (decl, MINIMUM_METHOD_BOUNDARY);
    }
```
The good news is I think I found where the alignment is being set, so
it can be addressed later if desired.

I stumbled upon this while cleaning up the patch, grokfndecl is just so
full of cruft it's crazy hard to reason about. There's more than one
block that I am near certain is completely dead code. I would like to
just ignore them for now but some of them unfortunately pertain to xobj
functions. I just don't feel good about putting in any hacks, but to
really get any modifications in here correct it would need to be
refactored much more than I should be doing in this patch.

Here's another example that I'm not sure how I want to address it.

~decl.cc:10331 grokfndecl
```
  int staticp = ctype && TREE_CODE (type) == FUNCTION_TYPE;
```
~decl.cc:10506 grokfndecl
```
  /* If this decl has namespace scope, set that up.  */
  if (in_namespace)
    set_decl_namespace (decl, in_namespace, friendp);
  else if (ctype)
    DECL_CONTEXT (decl) = ctype;
  else
    DECL_CONTEXT (decl) = FROB_CONTEXT (current_decl_namespace ());
```
And just a few lines down;
~decl.cc:10529
```
  /* Should probably propagate const out from type to decl I bet (mrs).  */
  if (staticp)
    {
      DECL_STATIC_FUNCTION_P (decl) = 1;
      DECL_CONTEXT (decl) = ctype;
    }
```

If staticp is true, ctype must have been non-null, and if ctype is
non-null, the context for decl should have been set in the second
block. So why was the code in the second block added?

commit f3665bd1111c1799c0421490b5e655f977570354
Author: Nathan Sidwell <nathan@acm.org>
Date:   Tue Jul 28 08:57:36 2020 -0700

    c++: Set more DECL_CONTEXTs
    
    I discovered we were not setting DECL_CONTEXT in a few cases, and
    grokfndecl's control flow wasn't making it clear that we were doing it
    in all cases.
    
            gcc/cp/
            * cp-gimplify.c (cp_genericize_r): Set IMPORTED_DECL's context.
            * cp-objcp-common.c (cp_pushdecl): Set decl's context.
            * decl.c (grokfndecl): Make DECL_CONTEXT setting clearer.

According to the commit, it was because it was not clear, which quite
frankly I can agree to, it just wasn't determined that the code below
is redundantly setting the context so it wasn't removed.

This puts me in a dilemma though, do I put another condition in that
code block for the xobj case even though the code is nearly dead? Or do
I give it a minor refactor for it to make a little more sense? If I add
to the code I feel like it's just going to add to the problem, while if
I give it a minor refactor it still won't look great and has a greater
chance of breaking something.

In this case I'm going to risk refactoring it, staticp is only used in
that 1 place so I will just rip it out. I am not concerned with decl's
type spontaneously changing to something that is not FUNCTION_TYPE, and
if it did I think there are bigger problems afoot.

I guess I'll know if I went too far with the refactoring when the patch
reaches you, do let me know about this one specifically though because
it took up a lot of my time trying to decide how to address it.

> > 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.
> 
> 
> Curious. It might also be a remnant of how older code dealt with how
> sometimes a member function changes between FUNCTION_TYPE and
> METHOD_TYPE during parsing.

Sounds plausible.

> > 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 would certainly expect it to be getting set already.

This being on my mind is partially what sent me down the rabbit hole
above, but yeah, it seems like it is.

> > 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.
> 
> 
> For a pointer-to-member, the class type is part of the type, so trying
> to remove it from the type doesn't sound like an improvement to me.
> Specifically, TYPE_PTRMEM_CLASS_TYPE refers to TYPE_METHOD_BASETYPE for
> a pointer to member function.

I suppose it's not a good idea given that other decl nodes use the same
field, but redundancies do feel icky to me. It makes stuff way harder
to reason about for me.

> > 
> > 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.
> 
> 
> Makes sense. I wouldn't expect access control to need specific changes.

I think I saw something that concerned me regarding this in grokfndecl,
but I agree that it shouldn't need specific changes, so I'm going to
assume I imagined it unless problems start to pop up :).

> No, that test has been pretty flaky for me, you can ignore it.

Noted.

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

All tests seemed to pass when applied to GCC14, but the results did
something funny where it said tests disappeared and new tests appeared
and passed. The ones that disappeared and the new ones that appeared
looked like they were identical so I'm not worrying about it. Just
mentioning it in case this is something I do need to look into.

Back to work now, thank you as always.

Alex