> > 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 goodness.
> > + 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.
AH, okay, I will go through and add them.
> > + pedwarn(DECL_SOURCE_LOCATION (xobj_parm), OPT_Wc__23_extensions,
>
>
> Missing space before (
Oops :)
> > + /* If */
>
>
> I think this comment doesn't add much. :)
I wish I remembered what I even meant to put here, oh well, must not
have been all that important. Oh wait, I remember it now, I was going
to note that the next element in the chain of declarators being null
seems to indicate that the declaration is a function type. I will
probably put that comment in again, it's on the line of commenting
exactly what the code does but it was cryptic to figure this out, I'm
not sure if I should lean towards including the comment here or not.
> > + 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 "
Noted.
> > + /* 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?
Unfortunately no, for the regular function case yeah but for member
function pointers it gets more complicated. Upon writing this and
revising 3 times referring to different cases where the user might have
meant something different, I'm realizing that a fixit is probably just
not appropriate for the pointer to member function case.
> > + /* Free function case,
> > + surely there is a better way to identify it? */
>
>
> Move these diagnostics down past where ctype gets set?
Sure, I'll see how it looks refactored that way.
> > + 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".
Noted.
> > + /* 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.
I'll be sure to ask him about it, thanks.
> > + 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 suppose so, I'll just do that then.
> > + /* 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)?
I've been reluctant to make any changes to parameters, but if you're
suggesting it I will consider it. Just replacing it with '(&name)'
seems really mediocre to me though. Yeah, sure, it's always going to be
syntactically valid but I worry that it wont be semantically correct
often enough.
Yeah, considering that I think I will leave it for now until I have
time to come up with a robust solution. I might still pass in the
location of the this token though and use error_at instead of error.
I'll see how I feel once I finish higher priority stuff.
> > + 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)
Ah yes, now I'm making the mistake that I hate to see around the code
base. I will definitely fix that.
I often wish these were member functions but a refactor supporting that
would not be trivial. Especially since node parameters are just tree
type, not the type you actually want to use them as, which would be an
even bigger refactor to support. Maybe one day perhaps.
> > +++ 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.
I know, I just wanted to be explicit about what was being tested. I
will remove it if inhibiting the other defaults is a significant issue.
> > +++ 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.
Yeah I had my doubt that -Wno-error=pedantic would work, so I suppose
the better option here is to just do { dg-options "" } yeah? I remember
it seemed to give me the behavior I wanted so I didn't worry about it.
To be clear, I should be replacing { dg-options "-Wno-error=pedantic" }
with { dg-options "" } and you would prefer I just remove { dg-options
"-pedantic-errors" } yeah?
> > + 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).
Thanks, I will get this in when I fix the other nearby problem (not
setting expr location if I remember correctly) you mentioned in your
other reply.
> > + 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
Thanks, this will speed things up considerably. I imagine this is also
where I would need to implement the warnings I mentioned regarding
by-value xobj member functions that are impossible to call.
I still have some other stuff to do today so it's unlikely I'll be able
to get anything (besides replying to e-mail) done. Thanks as always for
the detailed write up and pointing me in the right direction for the
missing diagnostics. Hopefully I can get started tomorrow if not
tonight.
Alex
