On Thu, Apr 28, 2022 at 12:15 PM Zebediah Figura zfigura@codeweavers.com wrote:
I think the struct packing is wrong, the SDK uses #pragma pack(1) here, which probably should use #include "pshpack1.h" / "poppack.h".
It doesn't make a difference in this case, though.
Hi,
Thanks for the review.
In the previous version pshpack1.h / poppack.h were included in usb100.h and ddk/wdm.h which was needed by ddk/winusbio.h. Since it doesn't make a difference in this case I suppose there's no reason to add it.
On Thu, Apr 28, 2022 at 8:33 AM Rémi Bernon rbernon@codeweavers.com wrote:
I have no idea what's the usual policy for how to write these. The signatures look alright, but are we still using LP*/P* types, or should it be expanded to pointers?
I'm not sure either, Wine is inconsistent in this regard. At least in winbase.h there's a lot of LP*/P* types.
Just curious what difference it makes compared to being expanded to pointers?
I see the previous patch version had more functions, and I can't really tell if it's better or not. Imho either add only the functions that are going to be used (so WinUsb_Free) or required by third-party programs to build, or add everything that the SDK public headers declare?
Anything in between seems arbitrary to me, but I have not much experience in adding stuff like this.
Yeah, I agree that either all the functions or just the one that's going to be used would be better.
The reason I removed the ddk/winusbio.h dependent functions is that some copyright concerns were raised for the winusbio.h file. It's a rather small file and I'm not sure how to add it in this case. Would rearranging the flags and structs be sufficient?
I'm not sure what the policy for headers is. IMO, I find it useful for public headers to be fully added, well the relevant parts to Wine that is. It saves developers time from having to add them later. Still, I'm fine with doing it either way.
-- Kind regards, Mohamad