Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()

[Enrico Weigelt, metux IT consult]

On 20.08.19 16:22, Pali RohÃr wrote:

Hi,

> > In that case, wouldn't a comment be more suitable for that ?
>
> And why to add comment if current state of code is more-readable and
> does not need it?

Readability is probably a bit subjective :p

With ongoing efforts of automatically identifying redundant code pathes,
the current situation causes the same discussion coming up over and over
again. Sooner or later somebody might get the idea to add a comment on
that line, that it's exactly as intented :o

OTOH, I'm unsure whether it's important to document that is particular
error path is unlikely, while we don't do it in thousands of other
places. IMHO, error pathes are supposed to be unlikely by nature,
otherwise we wouldn't call it an error situation ;-)

> People normally add comments to code which is problematic to understand
> or is somehow tricky, no so obvious or document how should code behave.

Yes, but isn't this case so obvious that it doesn't need any
documentation at all ? Is it so important to never ever forget that this
particular path is a rare situation ?

I might be wrong, but IMHO the likely()/unlikely() macros have been
introduced for optimization (better pipeline optimization for the
frequent cases).

> People first need to understand that static code analysis cannot work
> 100% reliably and always is needed to properly review changes.

Yes, but we see that this particular case is coming up again and again,
from different people, who can't know the background of this (nobody
can read the whole lkml and remember everything). This could be
prevented by adding a comment on that line, but then the macro call
just for documentation wouldn't be necessary anymore.

> And if the only reason for this change is to satisfy some static code
> analysis program,

Actually, it's more for people who're using the tools for the purpose
of tidying up the code and find potential problems. They need some
information to act on properly, which is currently missing ...

> would not it better to teach this program to no
> generate such false-positive results?

Okay, but how to do that practically ? There's no information in the
code which neither some tool nor any human could recognize this false
alarm. That information is just in your brain, now also in mine, and
the other folks who previously proposed the same change (assuming they
still recall it after years).

Blacklisting doesn't seem to be a good idea in the long run. Once going
that route, such blacklists grow quickly and get hard to maintain (we
now have two entirely separately maintained data sources that need to
be synced manually, when something changes).

I believe the code should be kinda self-documenting, so the reader
shouldn't need any detailed special knowledge to understand it. IMHO,
when such discussions on actual implementation details come up, the
code isn't expressive enough.

How can we improve the situation ?

Shall we consider introducing yet another macro (explicitly different
name) for stating/documenting such cases ?


--mtx

--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@xxxxxxxxx -- +49-151-27565287