Hi Abramo,

On Thu, 16 Dec 2021 at 05:44, Abramo Bagnara <abramo.bagnara@...> wrote:

Il 13/12/21 04:37, Nicolas Pitre ha scritto:

On Sun, 12 Dec 2021, Abramo Bagnara wrote:

Who in its right mind will believe that sprinkling typecasts around
would make the code any safer and understandable? And that's not
mentioning those _double_ typecasts being added too!!! We should instead
aim for cast _reduction_ to let the compiler warn more about type
mismatches and air the code, not add new ones!

Note: Sometimes double casts are necessary, but we do hide them behind
special accessors.

In the specific, double casts are needed in two situations:

1) A char argument to ctype predicates should be first converted to
unsigned char (as the C standard requires) and then converted to
int to avoid mixing of signed and unsigned. Said that, nothing
prevents us from using helpers that accept char argument or to
embed such double casts in, e.g.,

#define CTYPE_CHAR(c) ((int)(unsigned char)(c))

Unless we find a good place for this macro I'd be forced to leave double
cast. If it is preferred I can add a line of documentation before each
occurence.

And a real kicker is this change:

- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure

Who really thinks that the above is idiomatic and an improvement in code
understandability? It is not that MISRA does require it in this
particular case, but it certainly did skew the mindset of the person who
did that change.

I'd like very much you take a look to

https://github.com/Abramo-Bagnara/zephyr/commit/87a42d4185828fb1e651604b8ee878063fb6b08a

so to be sure I have intercepted correctly the community expectations.

Of course I cannot decide if the community wanted API is to return a
bool or to possibly return a nonzero error code, but if the intention is
the latter the commit accomplish that uniformingly and keeps integer and
boolean separate.

I think Nicolas makes some good points here.

Zephyr code is too different from Linux style for my liking, e.g. the
strange {} around single-line statements.

Things like 'if (rc != 0)' worsen readability also.

CTYPE_CHAR()...oh please no. Macros make the code harder to read. See
what Nicolas wrote, but if you must cast, cast.

Regards,
Simon

--
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...