RFC: Using `-Wno-unused-parameter` in Plasma projects

[Aleix Pol]

I agree that the status quo isn't ideal. Q_UNUSED() adds too much
relevance to the argument that happens to be not needed.

I think that the best most usable way is your b) where you comment out
the arguments.
I think the warning should be kept. The problem with c) is that we
pollute the namespace with an identifier that we don't use and in the
end I know I've solved issues at times noticing this warning.

That's all very subjective though, so take this all prefixed with a
big fat IMHO.

Cheers!
Aleix

On Wed, Oct 27, 2021 at 7:42 PM Vlad Zahorodnii <vlad.zahorodnii at kde.org> wrote:
>
> Hi,
>
> The main issue with -Wunused-parameter is that it makes development more
> painful. For example, say that you have an observer class and you need
> to override a dozen of methods to handle various events, but you don't
> really care about arguments... In every method, you would need to add
> Q_UNUSED() just to shut the compiler up. Besides more work, it produces
> more (boilterplate) code. There are ways to make code compact, e.g.
>
>    class ActivityObserver : public Observer
>    {
>    public:
>        void pointerEvent(const QPoint &pos, int timestamp) override
>        {
>            Q_UNUSED(pos)
>            Q_UNUSED(timestamp)
>            notifyActivity();
>        }
>
>        void buttonEvent(int button, ButtonState state, int timestamp)
> override
>        {
>            Q_UNUSED(pos)
>            Q_UNUSED(state)
>            Q_UNUSED(timestamp)
>            notifyActivity();
>        }
>    };
>
> (a) omit parameter names
>
>    class ActivityObserver : public Observer
>    {
>    public:
>        void pointerEvent(const QPoint &, int) override
>        {
>            notifyActivity();
>        }
>
>        void buttonEvent(int, ButtonState, int) override
>        {
>            notifyActivity();
>        }
>    };
>
> The advantages:
>
> * more compact code
>
> The disadvantages:
>
> * code is less readable
> * in case you need to use some parameter, you would need to look it up
> in the base class, which is not convenient
> * adds more work: you would need to copy method prototype and remove
> each argname manually
>
> (b) comment out argnames
>
>    class ActivityObserver : public Observer
>    {
>    public:
>        void pointerEvent(const QPoint &/*pos*/, int /*timestamp*/) override
>        {
>            notifyActivity();
>        }
>
>        void buttonEvent(int /*button*/, ButtonState /*state*/, int
> /*timestamp*/) override
>        {
>            notifyActivity();
>        }
>    };
>
> The advantages:
>
> * compact code
> * more readable than just removing argnames
>
> The disadvantages:
>
> * adds more work: you would need to copy method prototype and comment
> out each argname manually
>
>
> (c) disable -Wunused-parameter
>
>    class ActivityObserver : public Observer
>    {
>    public:
>        void pointerEvent(const QPoint &pos, int timestamp) override
>        {
>            notifyActivity();
>        }
>
>        void buttonEvent(int button, ButtonState state, int timestamp)
> override
>        {
>            notifyActivity();
>        }
>    };
>
> The advantages:
>
> * compact code
> * no need to manually remove or comment out argnames
>
> The disadvantages:
>
> * ?
>
> Besides adding more work and code, it's easy to forget to remove
> Q_UNUSED() once it actually becomes readable; it litters code with false
> information. Many compilers won't print a warning if an argument is used
> but it's marked as Q_UNUSED.
>
> One could argue that the main benefit of -Wunused-parameter is that it
> makes easy to spot when an argument becomes unused and thus it can be
> removed. But I doubt that it's ever been the case. Speaking from my
> personal experience, if I see an unused argument warning, my natural
> instinct is to add a Q_UNUSED() macro without analyzing every possible
> usage of that method or function.
>
> One could also argue that -Wunused-parameter may help to catch bugs
> caused by not using some parameter. Similar to the case above, the
> likelihood of that being true is slim. If an argument is unused, it's
> more likely that it will be marked as Q_UNUSED() without doing an in
> detail review.
>
> Note that this is not about -Wunused-variable. I believe that it's still
> very useful for the purpose of keeping code clean of unused variables.
>
> So my question is - would it be okay to disable -Wunused-parameter
> throughout all Plasma projects except maybe header-only libraries if
> there are any? If not, would it be acceptable to disable
> -Wunused-parameter per project instance?
>
> Cheers,
> Vlad