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

[Jason Merrill <jason@redhat.com>]

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.

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

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

>>> + /* 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.

> 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