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

[Jason Merrill <jason@redhat.com>]

On 10/18/23 07:46, waffl3x wrote:
>> Any progress on this, or do I need to coax the process along?  :)
> 
> Yeah, I've been working on it since the copyright assignment process
> has finished, originally I was going to note that on my next update
> which I had hoped to finish today or tomorrow. Well, in truth I was
> hoping to send one the same day that copyright assignment finished, but
> I found a nasty bug so I spent all day adding test cases for all the
> relevant overloadable operators. Currently, it crashes when calling a
> subscript operator declared with an explicit object parameter in a
> dependent context. I haven't looked into the fix yet, but I plan to.
> 
> Also, before I forget, what is the process for confirming my copyright
> assignment on your end? Do you just need to check in with the FSF to
> see if it went through? Let me know if there's anything you need from
> me regarding that.
> 
> Aside from the bug that's currently present in the first patch, I only
> have like 1 or 2 little things I want to change about it. I will make
> those few changes to patch 1, finish patch 2 (diagnostics) which will
> also include test cases for the new bug I found. After I am done that I
> plan on adding the things that are missing, because I suspect that
> looking into that will get me close to finding the fix for the crash.
> 
>> Hmm, is it? I see that clang thinks so, but I don't know where they get
>> that idea from. The grammar certainly allows it:
>>
>>> attribute-specifier-seqopt decl-specifier-seq declarator = initializer-clause
>>
>> and I don't see anything else that prohibits it.
> 
> You would be right for P0847R7, but CWG DR 2653 changed that. You can
> find the updated grammar in dcl.fct section 3 (subsection? I'm not
> really sure to be honest.)
> 
> I've also included a copy of the grammar here for your convenience.
> 
> https://eel.is/c++draft/dcl.fct#nt:parameter-declaration
> parameter-declaration:
>    attribute-specifier-seqopt thisopt decl-specifier-seq declarator
>    attribute-specifier-seqopt decl-specifier-seq declarator = initializer-clause
>    attribute-specifier-seqopt thisopt decl-specifier-seq abstract-declaratoropt
>    attribute-specifier-seqopt decl-specifier-seq abstract-declaratoropt = initializer-clause

Ah, yes, thanks.

>> I was thinking to set a TREE_LANG_FLAG on the TREE_LIST node.
> 
> I did figure this is originally what you meant, and I can still change
> it to go this route since I'm sure it's likely just as good. But I do
> recall something I didn't like in the implementation that nudged me
> towards using the purpose member instead. Either way, not a big deal. I
> think I just liked not having to mess with a linked list as I am not
> used to them as a data structure, it might have been that simple. :^)

I wouldn't expect to need any actual dealing with the linked list, just 
setting a flag in cp_parameter_declaration_list at the same point as the 
existing PARENTHESIZED_LIST_P flag.

But given CWG2653 as you pointed out, your current approach is fine.

> I will try to get something done today, but I was struggling with
> writing some of the tests, there's also a lot more of them now. I also
> wrote a bunch of musings in comments that I would like feedback on.
> 
> My most concrete question is, how exactly should I be testing a
> pedwarn, I want to test that I get the correct warning and error with
> the separate flags, do I have to create two separate tests for each one?

Yes.  I tend to use letter suffixes for tests that vary only in flags 
(and expected results), e.g. feature1a.C, feature1b.C.

> I'm just going to include the little wall I wrote in decl.cc regarding
> pedwarn, just in case I can't get this done tonight so I can get some
> feedback regarding it. On the other hand, it might just not be very
> relevant to this patch in particular as I kind of noted, but maybe
> there's some way to do what I was musing about that I've overlooked. It
> does end up a bit ranty I guess, hopefully that doesn't make it
> confusing.
> 
> ```
> /* I believe we should make a switch for this feature specifically,
>     I recall seeing discussion regarding enabling newer language
>     features when set to older standards. I would advocate for a
>     specific flag for each specific feature. Maybe they should all
>     be under an override flag? -foverride-dialect=xobj,ifconstexpr (?)
>     I dont think it makes sense to give each feature override it's own
>     flag. I don't recall what the consensus was around this discussion
>     either though.
>
>     For the time being it's controlled by pedantic. I am concerned that
>     tying this to pedantic going forward that one might want to enable
>     -pedantic-errors while also enabling select features from newer
>     dialects. It didn't look like this use case is supported to me.
> 
>     I suppose this will require redesign work to support, so for
>     the purposes of this patch, emitting a pedwarn seems correct.
>     I just don't like that it can't be suppressed on an individual
>     basis.  */
> if (xobj_parm && cxx_dialect < cxx23)
>    pedwarn(DECL_SOURCE_LOCATION (xobj_parm), OPT_Wpedantic, "");

Instead of OPT_Wpedantic, this should be controlled by 
-Wc++23-extensions (OPT_Wc__23_extensions)

If you wanted, you could add a more specific warning option for this 
(e.g. -Wc++23-explicit-this) which is also affected by 
-Wc++23-extensions, but I would lean toward just using the existing 
flag.  Up to you.

Jason