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

[Jason Merrill <jason@redhat.com>]

On 11/4/23 02:40, waffl3x wrote:
> 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)
> ```

So yes, it was for PMFs using the low bit of the pointer to indicate a 
virtual member function.  Since an xob memfn can't be virtual, it's 
correct for them to have the same alignment as a static memfn.

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

Removing the redundant DECL_CONTEXT setting seems appropriate, and 
changing how staticp is handled to reflect that xobfns can also have 
FUNCTION_TYPE.

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

That doesn't sound like a problem, but I'm curious about the specific 
output you're seeing.

Jason