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 / "name." but it would be
+ very difficult to get it perfect and I've been advised against
+ making imperfect fixits.
+ Perhaps it would be as simple as the replacements listed,
+ even if one is move'ing/forward'ing, if the replacement is just
+ done in the same place, it will be exactly what the user wants?
+ Even if this is true though, there's still a problem of getting the
+ context of the expression to find which tokens to replace.
+ I would really like for this to be possible though.
+ I will decide whether or not to persue this after review. */
You could pass the location of 'this' from the parser to
finish_this_expr and always replace it with (&name)?
+ tree xobj_parm = FUNCTION_DECL_CHECK (fn)->function_decl.arguments;
DECL_ARGUMENTS (fn)
+ tree parm_name = DECL_MINIMAL_CHECK (xobj_parm)->decl_minimal.name;
DECL_NAME (xobj_parm)
+++ b/gcc/testsuite/g++.dg/cpp23/explicit-obj-cxx-dialect-B.C
@@ -0,0 +1,7 @@
+// P0847R7
+// { dg-do compile { target c++20_down } }
+// { dg-options "-pedantic-errors" }
-pedantic-errors is the default in the testsuite if there is no dg-options.
+++ b/gcc/testsuite/g++.dg/cpp23/explicit-obj-cxx-dialect-C.C
@@ -0,0 +1,7 @@
+// P0847R7
+// { dg-do compile { target c++20_down } }
+// { dg-options "-Wno-error=pedantic" }
This has the same effect as { dg-options "" } since any dg-options
replaces the default -pedantic-errors.
Also, -Wno-error=pedantic does not turn off -pedantic-errors for
-Wc++23-extensions.
+ func_ptr_type xobj_mem_f_ptr = &xobj_member_f; // { dg-error "undetermined error"
"disallowing address of unqualified explicit object member function is not implemented yet" {
xfail *-*-* } }
This would be complaining in cp_build_addr_expr_1 if you end up taking
the address of an xobj fn without arg being an OFFSET_REF (with
PTRMEM_OK_P).
+ void f1() const; // { dg-note "previous declaration" "detecting redeclarations of
iobj member functions without a ref qualifier as xobj member functions is known to be broken"
{ xfail *-*-* } }
+ void f1(this S1 const&); // { dg-error "cannot be overloaded with" "detecting
redeclarations of iobj member functions without a ref qualifier as xobj member functions is known to be
broken" { xfail *-*-* } }
In add_method, the code
/* Compare the quals on the 'this' parm. Don't compare
the whole types, as used functions are treated as
coming from the using class in overload resolution. */
if (! DECL_STATIC_FUNCTION_P (fn)
&& ! DECL_STATIC_FUNCTION_P (method)
/* Either both or neither need to be ref-qualified for
differing quals to allow overloading. */
&& (FUNCTION_REF_QUALIFIED (fn_type)
== FUNCTION_REF_QUALIFIED (method_type))
&& (type_memfn_quals (fn_type) != type_memfn_quals (method_type)
|| type_memfn_rqual (fn_type) != type_memfn_rqual (method_type)))
continue;
needs adjustment for the new "corresponding object parameters".
Jason
