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

[waffl3x <waffl3x@protonmail.com>]

On Wednesday, November 22nd, 2023 at 2:38 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/22/23 15:46, waffl3x wrote:
> 
> > On Tuesday, November 21st, 2023 at 8:22 PM, Jason Merrill jason@redhat.com wrote:
> > 
> > > On 11/21/23 08:04, waffl3x wrote:
> > > 
> > > > /* Nonzero for FUNCTION_DECL means that this decl is a non-static
> > > > - member function. */
> > > > + member function, use DECL_IOBJ_MEMBER_FUNC_P instead. */
> > > > #define DECL_NONSTATIC_MEMBER_FUNCTION_P(NODE) \
> > > > (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> > > > 
> > > > +/* Nonzero for FUNCTION_DECL means that this decl is an implicit object
> > > > + member function. */
> > > > +#define DECL_IOBJ_MEMBER_FUNC_P(NODE) \
> > > > + (TREE_CODE (TREE_TYPE (NODE)) == METHOD_TYPE)
> > > 
> > > I was thinking to rename DECL_NONSTATIC_MEMBER_FUNCTION_P rather than
> > > add a new, equivalent one. And then go through all the current uses of
> > > the old macro to decide whether they mean IOBJ or OBJECT.
> > 
> > I figure it would be easier to make that transition if there's a clear
> > line between old versus new. To be clear, my intention is for the old
> > macro to be removed once all the uses of it are changed over to the new
> > macro. I can still remove it for the patch if you like but having both
> > and removing the old one later seems better to me.
> 
> 
> Hmm, I think changing all the uses is a necessary part of this change.
> I suppose it could happen before the main patch, if you'd prefer, but it
> seems more straightforward to include it.
> 
> > > > + else if (declarator->declarator->kind == cdk_pointer)
> > > > + error_at (DECL_SOURCE_LOCATION (xobj_parm),
> > > > + /* "a pointer to function type cannot "? */
> > > > + "a function pointer type cannot "
> > > > + "have an explicit object parameter");
> > > 
> > > "pointer to function type", yes.
> > > 
> > > > + /* The locations being used here are probably not correct. */
> > > 
> > > Why not?
> > 
> > I threw them in just so I could call inform, but it doesn't feel like
> > the messages should be pointing at the parameter, but rather at the
> > full type declaration. When I put those locations in I wasn't sure how
> > to get the full declaration location, and I'm still not 100% confident
> > in how to do it, so I just threw them in and moved on.
> 
> 
> That would be more precise, but I think it's actually preferable for the
> inform to have the same location as the previous error to avoid
> redundant quoting of the source.

Yeah that makes sense, I'll revise the comment with that rationale and
we can maybe revisit it later.

> > > Let's clear xobj_parm after giving an error in the TYPENAME case
> > 
> > I don't like the spirit of this very much, whats your reasoning for
> > this? We're nearly at the end of the scope where it is last used, I
> > think it would be more unclear if we suddenly set it to NULL_TREE near
> > the end. It raises the question of whether that assignment actually
> > does anything, or if we are just trying to indicate that it isn't being
> > used anymore, but I already made sure to declare it in the deepest
> > scope possible. That much should be sufficient for indicating it's
> > usage, no?
> 
> 
> Hmm, I think I poked at that and changed my mind, but forgot to delete
> the suggestion. Never mind.

Perfect.

> > > > if ((!methodp && !DECL_XOBJ_MEMBER_FUNC_P (decl))
> > > > || DECL_STATIC_FUNCTION_P (decl))
> > > 
> > > I think this can just be if (DECL_OBJECT_MEMBER_FUNC_P (decl)).
> > 
> > Alright, and going forward I'll try to make more changes that are
> > consistent with this one. With that said I'm not sure it can, but I'll
> > take a close look and if you're right I'll make that change.
> > 
> > > > if (TREE_CODE (fntype) == METHOD_TYPE)
> > > > ctype = TYPE_METHOD_BASETYPE (fntype);
> > > > + else if (DECL_XOBJ_MEMBER_FUNC_P (decl1))
> > > > + ctype = DECL_CONTEXT (decl1);
> > > 
> > > All of this can be
> > > 
> > > if (DECL_CLASS_SCOPE_P (decl1))
> > > ctype = DECL_CONTEXT (decl1);
> > > 
> > > I think I'm going to go ahead and clean that up now.
> > 
> > Sounds good to me, a lot of this stuff needs small cleanups and I'm
> > just concerned about making them too much.
> 
> 
> My cleanup of the ctype logic is in now.

I'll make sure to base the final patch off a newer commit then, I don't
think I'll do that right now because it takes a lot of time for me. I
still haven't gotten used to all the workflows with git so doing
anything with it takes a lot out of me. Also, I would have to rerun the
testsuite on the newer commit so I can get an accurate baseline which
takes a few hours.

> > > > + /* Error reporting here is a little awkward, if the type of the
> > > > + object parameter is deduced, we should tell them the lambda
> > > > + is effectively already const, or to make the param const if it is
> > > > + not, but if it is deduced and taken by value shouldn't we say
> > > > + that it's taken by copy and won't mutate?
> > > > + Seems right to me, but it's a little strange. */
> > > 
> > > I think just omit the inform if dependent_type_p.
> > 
> > Maybe I don't understand what a dependent type is as well as I thought,
> > but doesn't this defeat every useful case? The most common being an
> > xobj parameter of lambda type, which will always be deduced. Unless a
> > template parameter does not count as a dependent type, which is not
> > something I've ever thought about before.
> 
> 
> No, you're right. A template parameter is certainly dependent. I think
> the informs are fine as they are.

Okay sounds good, I still have to clean up in there for the other
reasons you noted, and because it just doesn't behave the way I want in
some cases still.

I think the logic I want is probably something like, is dependent,
unless it's a type template parameter. I'll figure it out, probably.

> > Mildly related, a lot of the stuff I hacked together with multiple
> > levels of accessing macros and predicates was due to not being able to
> > find a solution for what I needed. I think we would highly benefit from
> > better documentation of the accessors and predicates. I believe I've
> > seen some that appear to be duplicates, and some where they don't
> > appear to be implemented properly or match their description. If there
> > is such a document please direct me to it as I have spent an hour or so
> > each time I stumble on one of these problems.
> > 
> > In the section I wrote in add_method I spent I think about 3 hours
> > trying to figure out what combination of predicates and accessing
> > macros was correct for what I was trying to do. It gets pretty
> > convoluted to traverse, especially when the implementations of them are
> > rather involved. Finding non_reference was a big help for example, and
> > I stumbled across it by pure luck.
> > 
> > If we don't have this kind of documentation anywhere yet, I'm happy to
> > work on it next. I've spent a lot of time looking at it all so I feel
> > like I have a DECENT grasp on some of it. I think it would greatly
> > enhance the status quo, and might even help us figure out what needs to
> > be cleaned up, removed, replaced, etc.
> > 
> > Side side note, if said document does exist already, I either need to
> > learn how to search the wiki more effectively or it needs some sort of
> > improvement.
> 
> 
> There isn't really such a document; the comments are the main internals
> documentation, such as it is. I wonder about reorganizing cp-tree.h
> rather than creating a separate document that is likely to go out of
> date more quickly?
> 
> Jason

Yeah if it gets out of sync it's a problem, I'm not a big fan of
generating documentation from source usually, but perhaps this is a
good case for it. Annotating the macros and predicates with tags to
make it more searchable. The tags being the actual C++ keywords or
whatnot.

Obviously there's not too much time to think about it right now so I'll
just leave it at that for the moment, but some sort of mechanism like
this to aide in organization, de-duplication, and search-ability would
be the right way forward IMO. It will be less likely to get to a state
where it's in a dire need of reorganization that way.

One last thing on that, I've noticed there appears to still be some
pollution of gcc/tree.h with C++ constructs. Indeed, I checked really
quick just now and spotted a predicate for relating to virtual
functions/inheritance. I recognize that's a huge task to fully clean up
so I understand why it's still the case, but that should probably be
tackled at the same time.

/* Nonzero means that the derivation chain is via a `virtual' declaration.  */
#define BINFO_VIRTUAL_P(NODE) (TREE_BINFO_CHECK (NODE)->base.static_flag)

Back to work, thanks for the quick reply.

Alex