[riot-notifications] [RIOT-OS/RIOT] Refactor evtimer add del (#10717)

Marian Buschsieweke notifications at github.com
Wed Jul 3 11:21:38 CEST 2019


maribu commented on this pull request.



>  {
     DEBUG("evtimer: new event offset %" PRIu32 " ms\n", event->offset);
-    /* we want list->next to point to the first list element. thus we take the
-     * *address* of evtimer->events, then cast it from (evtimer_event_t **) to
-     * (evtimer_event_t*). After that, list->next actually equals
-     * evtimer->events. */
-    evtimer_event_t *list = (evtimer_event_t *)&evtimer->events;
-
-    while (list->next) {
-        evtimer_event_t *list_entry = list->next;
+    evtimer_event_t **list = &evtimer->events;

Let me explain why I think it is beneficial to avoid an extra layer of indirection if this can be done without pain: As part of my job I teach low level programming (among other stuff) to computer science students. From my experience at most every second student is able to understand the concept of pointers, as long as you remain at one level of indirection (pointer to foo). When you add one more layer (pointer to pointer to foo), at most one out of ten students will be able to understand the code.

While this is basically anecdotal evidence, I'm pretty sure that using 2 levels of indirection (pointer to pointer to foo) will give a lot of people a hard time trying to understand the code. There clearly are places where using pointers to pointers can make the code much shorter, more performant, or may just not be practical to implement with 1 level of indirection. But when there is no "pain" in using pointer to foo instead of pointer to pointer to foo, this will make a lot of peoples life easier ;-)

Let me sketch how this could be done with 1 level of indirection. (Untested code without having spend more than a second of thought.):

``` C
static void evtimer_add_event_to_list(evtimer_t *evtimer, evtimer_event_t *event)
{
    if (!evtimer->events || (event->offset < evtimer->events->offset)) {
        /* insert as first element */
        event->next = evtimer->events;
        evtimer->events = event;
    }
    else {
        evtimer_event_t *i;
        for (i = evtimer->events; i->next; i = i->next) {
            /* Set event offset relative to previous event */
            event->offset -= i->offset;

            if (event->offset < i->next->offset) {
                break;
            }
        }
        /* insert event between i and i->next */
        event->next = i->next;
        i->next = event;
    }

    if (event->next) {
        /* Updating offset of the follow-up to the newly inserted event to
           be relative to the newly inserted one */
        event->next->offset -= event->offset;
    }
}
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10717#discussion_r299858819
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190703/bdfc7452/attachment.html>


More information about the notifications mailing list