Re: [v1 2/2] winejoystick.drv/joystick_osx.c: sorts devices by location ID

[Ken Thomases]

On Jun 24, 2016, at 8:33 PM, David Lawrie  wrote:
> Fixes https://bugs.winehq.org/show_bug.cgi?id=38997
> for winmm joysticks

This should probably be a note and go below the "---" line, below.

Also, please adjust the subjects of these patches.  Take a look at how Alexandre changed the subjects of your previous commits.  Try to save him the effort.  The prefix should generally be just the DLL name without the name of the specific file (although changes to tests can have "/tests").  The first word after the colon should be capitalized.  The verb should be in the imperative mood ("Sort" vs. "Sorts").  If you want to include the information that this patch is for the Mac which is lost by removing the file name, add that to the subject (e.g. "Sort devices by location ID on the Mac").
> @@ -321,6 +321,34 @@ static CFIndex find_top_level(IOHIDDeviceRef hid_device, CFMutableArrayRef main_
> /**************************************************************************
> -
>                          find_osx_devices
>
> */

You've separated the function header comment above from the function it goes with.
> -
>
> +/*
> -
> - Helper to sort array by location ID since they are consistent across boots & launches
> - */

Use the function header comment format used elsewhere in the file.
> +static CFComparisonResult CFDeviceArrayComparatorFunction(const void *val1, const void *val2, void *context)

As with the previous patches, don't start your function names with "CF" which is for Core Foundation functions.  There's no reason to include "Function" in the name of a function.  Use lowercase-and-underscores.  Also, it should mention what it's comparing by or for.  For example, "device_location_comparator" similar to button_usage_comparator() elsewhere in this file.
> +{
> +#pragma unused(context)

Don't bother with that.
> - CFComparisonResult result = kCFCompareEqualTo;
> -
> - long loc1 = IOHIDDevice_GetLocationID((IOHIDDeviceRef)val1);
> - long loc2 = IOHIDDevice_GetLocationID((IOHIDDeviceRef)val2);

There's no need to cast void pointers to other pointer types.
> - if (loc1 < loc2) {
> -
>    result = kCFCompareLessThan;
> - } else if (loc1 > loc2) {
> -
>    result = kCFCompareGreaterThan;
> - }

If you're going to use braces around these blocks (which isn't necessary), put the braces on separate lines.
> +/*
> -
> - Helper to copy the CFSet to a CFArray
> - */
>
> +static void CFSetApplierFunctionCopyToCFArray(const void *value, void *context)

I know this name was copied from dinput, but it's still terrible. ;)
> +{
> - CFArrayAppendValue((CFMutableArrayRef)context, value);

Unneeded cast.
> @@ -363,20 +391,16 @@ static int find_osx_devices(void)
>     if (devset)
>     {
>         CFIndex num_devices, num_main_elements;
> -
>    const void** refs;
> -
>    CFArrayRef devices;
> -
>    CFMutableArrayRef devices;
>
>   num_devices = CFSetGetCount(devset);
> -
>    refs = HeapAlloc(GetProcessHeap(), 0, num_devices * sizeof(*refs));
> -
>    if (!refs)
> -
>    {
> -
>        CFRelease(devset);
> -
>        goto fail;
> -
>    }
>
> -
>    CFSetGetValues(devset, refs);
> -
>    devices = CFArrayCreate(NULL, refs, num_devices, &kCFTypeArrayCallBacks);
> -
>    HeapFree(GetProcessHeap(), 0, refs);
> -
>    devices = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks);

As a minor optimization, you can specify a capacity to the above call.
> -
>    /* copy devices from (unordered) set to array */
> -
>    CFSetApplyFunction(devset, CFSetApplierFunctionCopyToCFArray, (void *)devices);

Unneeded cast.
> -
>    /* now sort the array by location ID's */

Again, these comments don't add much and better names for the functions would make the code more self-explanatory.  It might be worthwhile to put a comment here or in the function header about _why_ you're sorting by location ID (i.e. to provide a stable order across runs).  That's a good general rule: comments shouldn't narrate what the code is doing, they should explain why it's doing it, or why it's doing it in a particular way, if that's non-obvious.
> -
>    CFArraySortValues(devices, CFRangeMake(0, num_devices), CFDeviceArrayComparatorFunction, NULL);
> -

-Ken