Address warnings related to casting of void* to 64

[erlingrj]

In this PR I address compiler warnings for our 32bit targets due to a casting from a void* to a uint64_t found in the pqueue implementation. It is discussed in #388.

I define the pqueue_pri_t to be a uintptr_t which is the safe way of interpreting a pointer as an integer. It will be 64 or 32 bit, depending on architecture. Since we still want the reaction index to be 64bit, I no longer use the pqueue_pri_t as the type of this field.

I dont fully understand the relationship between the pointers on the pqueue and the reaction-indices. It seems wierd that we should cast the pointers of the pqueue elements to integers in the first place.

[edwardalee]

This looks problematic to me. I think that the index field in reaction_t needs to be 64 bits regardless of the platform. The low-order 16 bits of this field are the level, and the remaining bits are the deadline. If we limit this field to 32 bits, then the deadline can only be 16 bits, which means that no deadline can be longer than 2^16 ns, or about 65us. If I'm understanding correctly, with these changes, the following function will return a 32-bit number on a 32-bit platform:

pqueue_pri_t get_reaction_index(void* reaction_t);

Note that if a deadline is given that is bigger than 2^16, then I think it will wrap around, which could lead to errors in reaction ordering.

With the index being 64 bits, deadlines are limited to about one hour, I think (2^48 ns).

A better alternative might be to change the reaction queue is a manner similar to what @byeonggiljun is doing for the event queue, where a priority is always a pointer and comparison functions are provided to compare reactions. This could eliminate the use of the index altogether and use the inferred deadline first for ordering and then the level when the inferred deadline is the same. This would also simplify the code.

[erlingrj]

The index field of reaction_t is still 64 bit. That particular function would work if we made it return index_t instead of pqueue_pri_t. I still don't understand the coupling between pqueue_pri_t and the reaction index. If I understand the pqueue implementation, then it is pointers to reactions that are put on the pqueue, not indices. The indices are gotten by dereferencing the pointers and accessing the index field.

[edwardalee]

Yes, updating the get_reaction_index function signature may be enough to fix this issue. If I understand correctly, the coupling between pqueue_pri_t and the reaction index was a premature optimization that enabled the use of this function for comparing priorities:

static int in_reverse_order(pqueue_pri_t thiz, pqueue_pri_t that) { return (thiz > that); }

If you look for pqueue_init calls, you will see that this function is provided most often for the cmppri (compare priorities) argument. I think the theory was that this would make pqueue faster, which may be true, but it comes at the expense of complexity, such as having to pack deadlines and levels into one pqueue_pri_t number and sorting the event queue by timestamps, ignoring microsteps. I doubt that there would be much performance impact if we simply provided a more specific cmppri function when we call pqueue_init. This is what @byeonggiljun is doing with the event queue.

[erlingrj]

Thanks for the explanation, I think the effort from @byeonggiljun makes sense here. But what is still puzzling to me is why we would want to cast a pointer to a reaction struct into a pqueue_pri_t. Does it ever make sense to treat those pointers as integers and compare them with each other? It is that cast that gives warnings for 32bit archs and what triggered me to change the pqueue_pri_t.

[edwardalee]

No, I don't think we ever want to directly compare pointers. When the priority is a pointer, the in_reverse_order function is useless. You have to provide a more useful comparison function to pqueue_init. The pqueue_tag.h and .c files provide examples, although @cmnrd has commented that the casts needed to use those without code duplication look suspicious to him. But I think it would be relatively easy to create a similar pqueue_reaction.h and .c that would implement its own cmppri function that doesn't require packing the inferred deadline and level into a single index. We could get rid of the index field.

[erlingrj]

Thanks for the information. It is clear that I need to take a close look at how the pqueue is used before making any changes. I will coordinate with @byeonggiljun if necessary. I am marking this PR as a draft, and I will come back to it.