Re: [riot-devel] Eliminating casts

[Kees Bakker]

PR per driver sounds good to me.

The required change in tcs37727 indeed involves a bit more. But I hope that
everyone agrees that you cannot change a const object into a non-const
object.

Take a look at the doxygen. The param[in] is misleading, the object will 
be modified.

Ha, it even says so in the description.

/**
 * @brief   Read sensor's data
 *
 * Besides an Autogain routine is called. If a maximum or minimum threshold
 * value of the channel clear is reached, then the gain will be changed
 * correspond to max or min threshold.
 *
 * @param[in]  dev device descriptor of sensor
 * @param[out] data    device sensor data, MUST not be NULL
 */
void tcs37727_read(const tcs37727_t *dev, tcs37727_data_t *data);


On 26-12-18 23:29, Joakim Nohlgård wrote:
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 >:


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.

8X8X---
void at86rf2xx_tx_exec(const at86rf2xx_t *dev)
{
 netdev_t *netdev = (netdev_t *)dev;
8X8X---

8X8X---
void kw2xrf_setup(kw2xrf_t *dev, const kw2xrf_params_t *params)
{
 netdev_t *netdev = (netdev_t *)dev;
8X8X---

8X8X---
static void _isr(netdev_t *netdev)
{
 ethos_t *dev = (ethos_t *) netdev;
 dev->netdev.event_callback((netdev_t*) dev,
NETDEV_EVENT_RX_COMPLETE);
8X8X---
Cast flipflop.

8X8X---
void tcs37727_read(const tcs37727_t *dev, tcs37727_data_t *data)
{
...
 tcs37727_trim_gain((tcs37727_t *)dev, tmpc);
8X8X---
Here tcs37727_trim_gain is actually modifying the const object. Bad.

8X8X---
 return cc2420_init((cc2420_t *)dev);
8X8X---
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


___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Eliminating casts

[Joakim Nohlgård]

A controversial idea is to change the implementation to C++ and make the
driver a proper class with inheritance and utilize polymorphism with
dynamic casts.

/Joakim

Den tor 27 dec. 2018 21:51 skrev Kees Bakker :

> On 27-12-18 13:54, Kaspar Schleiser wrote:
> > Hi,
> >
> > On 12/26/18 11:16 PM, Kees Bakker wrote:
> >> Suppose I make a Pull Request to eliminate casts, would that be picked
> up?
> > Always welcome! +1 on Joakim's hint to keep the PR's small.
> Sure
> >> void at86rf2xx_tx_exec(const at86rf2xx_t *dev)
> >> {
> >>  netdev_t *netdev = (netdev_t *)dev;
> > What would be the way to go here? 'netdev_t netdev = >netdev;'?
> Yes, please. It's already done in several drivers.
> > In this case, it won't work. Would need to recurse into
> > netdev_ieee802154_t, like 'netdev_t *netdev = >netdev.netdev;'.
> >
> > That might be less error.prone, but more confusing. Unfortunately C is
> > not much help here.
> Is it more confusing? I don't think so. Personally, I prefer the
> >netdev.netdev
> above the (netdev_t *)dev.
> >> 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.
> >> * ...
> > * use dereferenced superclass field instead of "blind" cast
> >
> > (the netdev case above).
> >
> > Kaspar
> > ___
> > 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
>
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Eliminating casts

[Kees Bakker]

On 27-12-18 13:54, Kaspar Schleiser wrote:

Hi,

On 12/26/18 11:16 PM, Kees Bakker wrote:

Suppose I make a Pull Request to eliminate casts, would that be picked up?

Always welcome! +1 on Joakim's hint to keep the PR's small.

Sure

void at86rf2xx_tx_exec(const at86rf2xx_t *dev)
{
     netdev_t *netdev = (netdev_t *)dev;

What would be the way to go here? 'netdev_t netdev = >netdev;'?

Yes, please. It's already done in several drivers.

In this case, it won't work. Would need to recurse into
netdev_ieee802154_t, like 'netdev_t *netdev = >netdev.netdev;'.

That might be less error.prone, but more confusing. Unfortunately C is
not much help here.
Is it more confusing? I don't think so. Personally, I prefer the 
>netdev.netdev

above the (netdev_t *)dev.

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.
* ...

* use dereferenced superclass field instead of "blind" cast

(the netdev case above).

Kaspar
___
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

Re: [riot-devel] Eliminating casts

[Kaspar Schleiser]

Hi,

On 12/26/18 11:16 PM, Kees Bakker wrote:
> Suppose I make a Pull Request to eliminate casts, would that be picked up?

Always welcome! +1 on Joakim's hint to keep the PR's small.

> void at86rf2xx_tx_exec(const at86rf2xx_t *dev)
> {
>     netdev_t *netdev = (netdev_t *)dev;

What would be the way to go here? 'netdev_t netdev = >netdev;'?

In this case, it won't work. Would need to recurse into
netdev_ieee802154_t, like 'netdev_t *netdev = >netdev.netdev;'.

That might be less error.prone, but more confusing. Unfortunately C is
not much help here.

> 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.
> * ...

* use dereferenced superclass field instead of "blind" cast

(the netdev case above).

Kaspar
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] RIOT-OS assembly @ 35C3

[Martine Lenders]

Hi,
In case you are searching for us. We were switched around a little:
https://35c3.c3nav.de/l/c:0:438.09:481.89/@0,439.08,482.83,5

Kind Regards,
Martine

Am Di., 25. Dez. 2018, 21:10 hat Martine Lenders 
geschrieben:

> Oops forgot the link: https://35c3.c3nav.de/l/riotos/
>
> Am Di., 25. Dez. 2018, 21:07 hat Martine Lenders 
> geschrieben:
>
>> Hi everyone,
>>
>> We now have a location for the RIOT assembly @ 35C3. See you there!
>>
>> Regards,
>> Martine
>>
>
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel