Re: [PATCH v4 2/2] c++: Diagnostics for P0847R7 (Deducing this) [PR102609]
> > I had wanted to write about some of my frustrations with trying to
> > write a test for virtual specifiers and errors/warnings for
> > shadowing/overloading virtual functions, but I am a bit too tired at
> > the moment and I don't want to delay getting this up for another night.
> > In short, the standard does not properly specify the criteria for
> > overriding functions, which leaves a lot of ambiguity in how exactly we
> > should be handling these cases.
>
>
> Agreed, this issue came up in the C++ committee meeting today. See
>
> https://cplusplus.github.io/CWG/issues/2553.html
> https://cplusplus.github.io/CWG/issues/2554.html
>
> for draft changes to clarify some of these issues.
Ah I guess I should have read all your responses before sending any
back instead of going one by one. I ended up writing about this again I
think.
struct B {
virtual void f() const {}
};
struct S0 : B {
void f() {}
};
struct S1 : B {
void f(this S1&) {}
};
I had a bit of a debate with a peer, I initially disagreed with the
standard resolution because it disallows S1's declaration of f, while
S0's is an overload. I won't bore you with all the details of going
back and forth about the proposed wording, my feelings are mostly that
being able to overload something with an iobj member function but not
being able to with an xobj member function was inconsistent. He argued
that keeping restrictions high at first and lowering them later is
better, and overloading virtual functions is already not something you
should really ever do, so he was in favor of the proposed wording.
In light of our debate, my stance is that we should implement things as
per the proposed wording.
struct B {
virtual void foo() const&;
};
struct D : B {
void foo(this int);
};
This would be ill-formed now according to the change in wording. My
laymans interpretation of the semantics are that, the parameter list is
the same when the xobj parameter is ignore, so it overrides. And since
it overrides, and xobj member functions are not allowed to override, it
is an error.
To be honest, I still don't understand where the wording accounts for
the qualifiers of B::f, but my friend assured me that it is.
> > The standard also really poorly
> > specifies things related to the implicit object parameter and implicit
> > object argument which also causes some trouble. Anyhow, for the time
> > being I am not including my test for diagnostics related to a virtual
> > specifier on xobj member functions. I can't get it to a point I am
> > happy with it and I think there will need to be some discussion on how
> > exactly we want to handle that.
>
>
> The discussion might be easier with the testcase to refer to?
I'm pretty clear on how to proceed now. My biggest issue with the tests
is I didn't know what combination of errors to emit, whether to warn
for overriding, etc. There is still a bit of an issue on how to emit
errors for virtual specifiers on xobj member functions, it gets noisy
quickly, but maybe we do just emit all of them? This aside, what should
be done here is clear now.
Somewhat related is some warnings I wanted to implement for impossible
to call by-value xobj member functions. Ones that involve an unrelated
type get dicey because people could in theory have legitimate use cases
for that, P0847R7 includes an example of this combining recursive
lambdas and the overload pattern to create a visitor. However, I think
it would be reasonable to warn when a by-value xobj member function can
not be called due to the presence of overloads that take references.
Off the top of my head I don't recall how many overloads it would take
to prevent a by-value version from being called, nor have I looked into
how to implement this. But I do know that redeclarations of xobj member
functions as iobj member functions (and vice-versa) are not properly
identified when ref qualifiers are omitted from the corresponding (is
this the correct term here?) iobj member function.
> > I was fairly lazy with the changelog and commit message in this patch
> > as I expect to need to do another round on this patch before it can be
> > accepted. One specific question I have is whether I should be listing
> > out all the diagnostics that were added to a function. For the cases
> > where there were only one diagnostic added I stated it, but for
> > grokdeclarator which has the majority of the diagnostics I did not. I
> > welcome input here, really I request it, because the changelogs are
> > still fairly difficult for me to write. Hell, the commit messages are
> > hard to write, I feel I went overboard on the first patch but I guess
> > it's a fairly large patch so maybe it's alright? Again, I am looking
> > for feedback here if anyone is willing to provide it.
>
>
> ChangeLog entries are very brief summaries of the changes, there's
> absolutely no need to enumerate multiple diagnostics. If someone wants
> more detail they can look at the patch.
Noted, thank
Re: [PATCH v4 2/2] c++: Diagnostics for P0847R7 (Deducing this) [PR102609]
On 11/5/23 10:06, waffl3x wrote: I had wanted to write about some of my frustrations with trying to write a test for virtual specifiers and errors/warnings for shadowing/overloading virtual functions, but I am a bit too tired at the moment and I don't want to delay getting this up for another night. In short, the standard does not properly specify the criteria for overriding functions, which leaves a lot of ambiguity in how exactly we should be handling these cases. Agreed, this issue came up in the C++ committee meeting today. See https://cplusplus.github.io/CWG/issues/2553.html https://cplusplus.github.io/CWG/issues/2554.html for draft changes to clarify some of these issues. The standard also really poorly specifies things related to the implicit object parameter and implicit object argument which also causes some trouble. Anyhow, for the time being I am not including my test for diagnostics related to a virtual specifier on xobj member functions. I can't get it to a point I am happy with it and I think there will need to be some discussion on how exactly we want to handle that. The discussion might be easier with the testcase to refer to? I was fairly lazy with the changelog and commit message in this patch as I expect to need to do another round on this patch before it can be accepted. One specific question I have is whether I should be listing out all the diagnostics that were added to a function. For the cases where there were only one diagnostic added I stated it, but for grokdeclarator which has the majority of the diagnostics I did not. I welcome input here, really I request it, because the changelogs are still fairly difficult for me to write. Hell, the commit messages are hard to write, I feel I went overboard on the first patch but I guess it's a fairly large patch so maybe it's alright? Again, I am looking for feedback here if anyone is willing to provide it. ChangeLog entries are very brief summaries of the changes, there's absolutely no need to enumerate multiple diagnostics. If someone wants more detail they can look at the patch. + if (xobj_func_p && (quals || rqual)) + inform (DECL_SOURCE_LOCATION (DECL_ARGUMENTS (decl)), + "explicit object parameter declared here"); When you add an inform after a diagnostic, you also need to add an auto_diagnostic_group declaration before the error so that they get grouped together for JSON/SARIF diagnostic output. This applies to a lot of the diagnostics in the patch. + pedwarn(DECL_SOURCE_LOCATION (xobj_parm), OPT_Wc__23_extensions, Missing space before ( + /* If */ I think this comment doesn't add much. :) + else if (declarator->declarator->kind == cdk_ptrmem) + error_at (DECL_SOURCE_LOCATION (xobj_parm), + "a member function pointer type " + "cannot have an explicit object parameter"); Let's say "a pointer to member function type " + /* Ideally we should synthesize the correct syntax + for the user, perhaps this could be added later. */ Should be pretty simple to produce an add_fixit_remove() for the 'this' token here? + /* Free function case, + surely there is a better way to identify it? */ Move these diagnostics down past where ctype gets set? + else if (decl_context == NORMAL +&& (in_namespace +|| !declarator->declarator->u.id.qualifying_scope)) + error_at (DECL_SOURCE_LOCATION (xobj_parm), + "a free function cannot have " + "an explicit object parameter"); Let's say "non-member function". + /* Ideally we synthesize a full rewrite, at the moment +there are issues with it though. +It rewrites "f(S this & s)" correctly, +but fails to rewrite "f(const this S s)" correctly. +It also does not handle "f(S& this s)" correctly at all. David Malcolm would be the one to ask for advice about fixit tricks, if you want. +It's also possible we want to wait and see if the parm +could even be a valid xobj parm as it might be confusing +to the user to see an error, fix it, and then see another +error for something new. I don't see how that applies here; we don't bail out after this error, so we should continue to give any other needed errors. + /* If default_argument is non-null token should always be the +the location of the `=' token, this is brittle code though +and should be rectified in the future. */ It would be easy enough to add an eq_token variable? + /* I can imagine doing a fixit here, suggesting replacing +this / *this / this-> with / name / "name." but it
[PATCH v4 2/2] c++: Diagnostics for P0847R7 (Deducing this) [PR102609]
Bootstrapped and tested on x86_64-linux with no regressions. Finally, the fabled diagnostics patch. I would like to note really quickly that there was never a v2 and v3 of this patch, only the first of these 2 had those versions. Originally I had planned to revise this patch alongside the first but it just didn't happen. Anyhow, I decided to match the version of this second patch to the current first patch to avoid any confusion. With that out of the way, I feel mostly okay about the code in this patch, but I have a feeling it will need a revision, especially with the large amounts of comments I left in. At the very least I expect to need to pull those out before the patch can be accepted. I had wanted to write about some of my frustrations with trying to write a test for virtual specifiers and errors/warnings for shadowing/overloading virtual functions, but I am a bit too tired at the moment and I don't want to delay getting this up for another night. In short, the standard does not properly specify the criteria for overriding functions, which leaves a lot of ambiguity in how exactly we should be handling these cases. The standard also really poorly specifies things related to the implicit object parameter and implicit object argument which also causes some trouble. Anyhow, for the time being I am not including my test for diagnostics related to a virtual specifier on xobj member functions. I can't get it to a point I am happy with it and I think there will need to be some discussion on how exactly we want to handle that. I was fairly lazy with the changelog and commit message in this patch as I expect to need to do another round on this patch before it can be accepted. One specific question I have is whether I should be listing out all the diagnostics that were added to a function. For the cases where there were only one diagnostic added I stated it, but for grokdeclarator which has the majority of the diagnostics I did not. I welcome input here, really I request it, because the changelogs are still fairly difficult for me to write. Hell, the commit messages are hard to write, I feel I went overboard on the first patch but I guess it's a fairly large patch so maybe it's alright? Again, I am looking for feedback here if anyone is willing to provide it. I've written more than I want here, so I'll wrap this e-mail up and go to bed. I am very happy to be getting close to a final product here. Hopefully if all goes well I'll be able to fit in the final missing features before feature lock hits. AlexFrom c8e8155a635fab7f326d0ad32326da352d7c323e Mon Sep 17 00:00:00 2001 From: waffl3x Date: Sun, 5 Nov 2023 05:17:18 -0700 Subject: [PATCH 2/2] c++: Diagnostics for C++23 P0847R7 (Deducing this) [PR102609] This patch adds diagnostics for various ill-formed code related to xobj member functions. Some of the code in here leaves something to be desired, but the majority of cases should be handled. I opted to add a new TFF flag despite only using it in a single place, other solutions seemed non ideal and there are plenty of bits left. Some of the diagnostics are more scattered around than I would like, perhaps this could be refactored in the future, especially those in grokfndecl. PR c++/102609 gcc/cp/ChangeLog: PR c++/102609 Diagnostics for C++23 P0847R7 - Deducing this. * cp-tree.h (TFF_XOBJ_FUNC): Define. * decl.cc (grokfndecl): Diagnose cvref-qualifiers on an xobj member function. (grokdeclarator): Diagnostics * error.cc (dump_function_decl): For xobj member function add TFF_XOBJ_FUNC bit to dump_parameters flags argument. (dump_parameters): When printing xobj member function's params add "this" to the first param. (function_category): Say so when in an xobj member function. * parser.cc (cp_parser_decl_specifier_seq): Diagnose incorrectly positioned "this" specifier. (cp_parser_parameter_declaration): Diagnose default argument on xobj params. * semantics.cc (finish_this_expr): Diagnose uses of "this" in body of xobj member function. gcc/testsuite/ChangeLog: PR c++/102609 Diagnostics for C++23 P0847R7 - Deducing this. * g++.dg/cpp23/explicit-obj-cxx-dialect-A.C: New test. * g++.dg/cpp23/explicit-obj-cxx-dialect-B.C: New test. * g++.dg/cpp23/explicit-obj-cxx-dialect-C.C: New test. * g++.dg/cpp23/explicit-obj-cxx-dialect-D.C: New test. * g++.dg/cpp23/explicit-obj-cxx-dialect-E.C: New test. * g++.dg/cpp23/explicit-obj-diagnostics1.C: New test. * g++.dg/cpp23/explicit-obj-diagnostics2.C: New test. * g++.dg/cpp23/explicit-obj-diagnostics3.C: New test. * g++.dg/cpp23/explicit-obj-diagnostics4.C: New test. * g++.dg/cpp23/explicit-obj-diagnostics5.C: New test. * g++.dg/cpp23/explicit-obj-diagnostics6.C: New test. * g++.dg/cpp23/explicit-obj-diagnostics7.C: New test. Signed-off-by: waffl3x --- gcc/cp/cp-tree.h | 5 +- gcc/cp/decl.cc| 133 ++--- gcc/cp/error.cc | 8 +-
