On 23/04/15 16:08, Derek Foreman wrote: > On 23/04/15 06:26 AM, Tom Hacohen wrote: >> Hey, >> >> Thanks for the fix. I don't know this code, but wouldn't it be cleaner >> to just define the variable win as uint64_t? It'll remove all of the >> ifdefs, and at worst will require some casts. >> >> Feels much safer to me. >> >> Also, I wonder where else we have a similar issue, and why gcc/coverity >> haven't warned us about truncating the value (or maybe they have and we >> just ignored them). > > Hmm, I think it would get rid of the ifdefs, yes. > > we'd still need: > if (type == E_PIXMAP_TYPE_WL) > ec = e_pixmap_find_client(type, win); > else > ec = e_pixmap_find_client(type, (Ecore_X_Window) win); > > since we're use varargs to defeat type safety the cast must match what > _e_pixmap_find expects based on type. > > Mike just replaced the way the IDs are generated for wayland windows, > and under the new method they're uintptr_t, which is the same as > Ecore_Window, so we can probably simplify this a bit further now. > > Actually, I think there's still a bug here, because (for X) we're > currently passing an Ecore_Window which is a uintptr_t and we're > expecting an Ecore_X_Window which is a uint32_t? > > None of this feels at all safe to me anyway, I'd love to see the varargs > go away and be replaced by a struct or a union that can hold both types... > > gcc can't tell us anything's wrong because we're intentionally using > varargs to defeat type safety.
As I said, I don't know this code, so I had no idea about the varargs, but based on your description: this is horrible. This is a sure way to introduce bugs. Either have a type that's big enough to hold everything (probably better), a consistent type or a union (which is almost the same as the first option). This needs to be fixed asap. :( -- Tom. ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
