search

contrem / arduino-timer

Non-blocking library for delaying function calls
BSD 3-Clause "New" or "Revised" License
334 stars 50 forks source link

src:arduino-timer: Cast t to Task, not uintptr_t #25

Closed skrobinson closed 3 years ago

skrobinson commented 3 years ago

The typedef for Task was changed in the same commit where this cast was added. Probably just an oversight.

Signed-off-by: Sean Robinson sean.robinson@scottsdalecc.edu

skrobinson commented 3 years ago

Does that mean task_id is doing an incorrect cast?

cancel(Task) tries to match the Task to a known struct task in the tasks array. I assumed that task_id returning (the equivalent of) (Task)t ^ t->id meant the same operation should be used for matching in cancel(Task).

If Task is changed to other than uintptr_t in the future, task_id and cancel(Task), as currently written, will be doing different calculations, right?

contrem commented 3 years ago

Does that mean task_id is doing an incorrect cast?

task_id is converting to a Task, so we cast to a Task.

cancel(Task) tries to match the Task to a known struct task in the tasks array. I assumed that task_id returning (the equivalent of) (Task)t ^ t->id meant the same operation should be used for matching in cancel(Task).

It's the same operation (^), but the result is logically different. In task_id, it converts a struct task * to a Task, in cancel it converts a Task to a struct task *, but for comparison cast to uintptr_t. Casting t to a Task in cancel would not be correct, because the comparison is between struct task *s. It would also be correct to cast the xor result to struct task * instead of casting t to uintptr_t, but it was shorter to write the uintptr_t cast.

If Task is changed to other than uintptr_t in the future, task_id and cancel(Task), as currently written, will be doing different calculations, right?

They're already different. It perhaps would have been clearer to create a from_task_id function (and name task_id to_task_id instead), but the from_task_id operation currently only happens in cancel. As written, a change to Task should give compiler errors for invalid operands to xor, flagging both locations.

Hope this helps it make sense.

skrobinson commented 3 years ago

the comparison is between struct task *s.

Thank you. This is the point I was misunderstanding.