I agree with your arguments. Placing casts where they are not needed may
introduce errors in a future refactoring where a variable changes type.
If you want your changes merged as smoothly as possible you should open a
separate PR for each module you modify. It will make the review much
easier. Stuff like the cc2420 change is going to get merged right away
pretty much since it's obvious that there are no side effects.
The tcs37727 change requires more thought for the correct change to make
and may lead to some review comments.
Best regards,
Joakim
Den ons 26 dec. 2018 23:17 skrev Kees Bakker <k...@sodaq.com>:
> Hey,
>
> In general, I'm not happy with casts when they are not really needed.
> A cast takes away an opportunity for the compiler to check things.
> Sometimes a cast is there to get rid of "const". That's not good.
> Sometimes a cast is there to get rid of "volatile". That's not good either.
>
> Suppose I make a Pull Request to eliminate casts, would that be picked up?
> Who would pick them up? Who decides if the PR is valid? What the general
> opinion about these sorts of PRs?
>
> If you want examples, here are some, from various drivers.
>
> ----------------------------8X------------8X-------------------
> void at86rf2xx_tx_exec(const at86rf2xx_t *dev)
> {
> netdev_t *netdev = (netdev_t *)dev;
> ----------------------------8X------------8X-------------------
>
> ----------------------------8X------------8X-------------------
> void kw2xrf_setup(kw2xrf_t *dev, const kw2xrf_params_t *params)
> {
> netdev_t *netdev = (netdev_t *)dev;
> ----------------------------8X------------8X-------------------
>
> ----------------------------8X------------8X-------------------
> static void _isr(netdev_t *netdev)
> {
> ethos_t *dev = (ethos_t *) netdev;
> dev->netdev.event_callback((netdev_t*) dev, NETDEV_EVENT_RX_COMPLETE);
> ----------------------------8X------------8X-------------------
> Cast flipflop.
>
> ----------------------------8X------------8X-------------------
> void tcs37727_read(const tcs37727_t *dev, tcs37727_data_t *data)
> {
> ...
> tcs37727_trim_gain((tcs37727_t *)dev, tmpc);
> ----------------------------8X------------8X-------------------
> Here tcs37727_trim_gain is actually modifying the const object. Bad.
>
> ----------------------------8X------------8X-------------------
> return cc2420_init((cc2420_t *)dev);
> ----------------------------8X------------8X-------------------
> In this case, dev is already of that type.
>
> In the Coding Conventions I like to see something written about casts.
> I'm not sure exactly about the wording. Here is an attempt.
>
> Casts
> * Try to avoid casts (a bit vague, but it should get people's attention)
> * Introduce helper variables to avoid multiple casts within a function
> * Don't cast a const pointer into a non-const pointer without an
> explanation in a comment.
> * Don't cast a pointer to a volatile object dropping the volatile without
> an
> explanation in a comment.
> * ...
> --
> Kees
> _______________________________________________
> devel mailing list
> devel@riot-os.org
> https://lists.riot-os.org/mailman/listinfo/devel
>
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
