C++ Logo

std-proposals

Advanced search

Re: [std-proposals] Detect non overriden function

From: Jason McKesson <jmckesson_at_[hidden]>
Date: Mon, 21 Mar 2022 22:12:15 -0400
On Mon, Mar 21, 2022 at 1:26 PM organicoman via Std-Proposals
<std-proposals_at_[hidden]> wrote:
>
> So you want the compiler to emit a warning that you're calling a method that
> was not overriden and was not meant to be overridden?
> No.
>
> Are you sure you
> understand the documentation of the class you're trying to use in the first
> place?
> Yes,

When you started the conversation, you thought this function was
`virtual`. You do understand that this makes it difficult for us to
believe you when you say that you understand the class, yes?

> If QProcess receives writes on its standrdError channel, and you set your channel to be standardOutput, calling readAll won't return a thing.

How do you know that this is not precisely the behavior the user
wants? Why should a user, who is using the class's interface
correctly, get a warning for it?

> That's why, i guess, they added two methods:
> readAllStandardError
> readAllStandardOutput
> Which both stores the current write channel switch to the appropriate channel then call readAll, then restore back the old channel.
>
> And since as an end user, I'm allowed to call readAll, which is available thru the base class,
> From its name, it sounds that it reads anything available no matter what channel you are on. Which obviously does not.

So now it's no longer about the relationships of base and derived
class interfaces. It seems your core problem is *name* of this
particular function. That if the base class function had been called
`readAllFromDefault`, you wouldn't have a problem with it.

To the extent that `readAll` is not the best name, this is purely a
naming convention problem. You don't solve naming problems with
language features or even static analysis tools. You solve it by
picking a better API.

> (I picked up QProcess::readAll, for lack examples, i didn't suspect it was not virtual, which is another reason to be notified)

But it isn't a reason to be notified. A derived class is a base class;
that's what it *means* to be a derived class. The base class interface
is, and of a right ought to be, considered 100% as valid as the
derived class's interface. And in QProcess, this is the case, as far
as the intent of the developers is concerned.

> But here's the thing: the derived class's implementer makes that choice, not
> you as a user. There's no reason for you to get a warning, if the class
> implementer has done their job properly. If you think that they haven't and
> that there's a bug, that's a different story, but then there s little the
> standard and the compilers can help with.
>
> No, it can help.
> Help the implementer communicate his intentions using warnings, annotations, attributes, to expose his design to the users, get feedback, refine the implementation.
> No one can guess all the scenarios of usage of his library.

But we've already established that "the implementer" in this case (ie:
Qt's developers) *wants* the API this way. This wasn't designed by
accident. They intend and expect users to use the base class interface
even when they knowingly use the derived class.

That is, if the developers had the ability to issue warnings if you
used `readAll` from the derived class, they *would not* use that
ability. So even if what you wanted was available, it wouldn't solve
this "problem" because the developers simply do not agree that it is a
"problem".

Received on 2022-03-22 02:12:38