[PATCH] to fix 3 of 3 warnings generated by the Clang Static Analyzer

[rogueresearch]

Hello,

The Clang Static Analyzer (​http://clang-analyzer.llvm.org/) produces 3 warnings building libusb on Mac OS X 10.6.

Attached is the HTML output of the analyzer, and a fix for the warnings. I couldn't figure out how to make a .patch file with git, so I've just attached the 2 files I changed.

I have only tested that this compiles, but the changes are trivial.

[rogueresearch]

patch and analyzer results

[rogueresearch]

patch in .patch format

[dsd]

Thanks for the patch. Could you include an explanation of the changes?

I don't get the io.c change - what does it do, and why?

The CFRunLoopGetCurrent() bit looks like apples vs oranges, or is there a reason why we don't want to call it so often?

And as for the final change, do we really want to ignore the error condition? Or should we actually be returning failure at that point?

[rogueresearch]

All the changes are to fix warnings. The attached zip has several HTML files that show the warnings.

The io.c change solves the warning about a value being assigned to variable and subsequently never being used. Casting a variable to void like that is a common and portable way to say, "yes, I know I'm not using it".

The CFRunLoopGetCurrent() change solves a warning about unbalanced retain/release. The analyzer is not smart enough to realise that CFRunLoopGetCurrent () returns the same thing both times. The change is good because then we're not relying on it returning the same thing.

The last one also fixes a warning about a stored value never being used. So I use it by logging it as an error.

[rogueresearch]

So dsd, will these changes be merged in?

[rogueresearch]

I just checked and this patch never seems to have been incorporated... is further clarification or improvement needed, or...? Thanks.

[stuge]

I think the Darwin parts are fine but we should be able to do something nicer for io.c.

[rogueresearch]

stuge, thanks for looking. Any suggestion for 'something nicer' for io.c? I don't really know what the code is doing, so I hesitate to propose anything more aggressive. The assignment to 'first' is indeed useless if '#ifdef USBI_TIMERFD_AVAILABLE' is false.

[dsd]

I think the io.c thing can just be ignored. The compiler is smart enough to not waste its time on it. Me too. :)

[rogueresearch]

I'm surprised by the resistance against fixing a small warning, but that's fine. :) How about the other file? Will you apply that patch?

[dsd]

just needs an ack from Nathan, the darwin maintainer

[hjelmn]

On the CFRunLoopGetCurrent part of the patch: Since the code is correct as is I would prefer that clang fix the bug on their end. That said, if there is some value in clang not printing an warning here its not that big of a deal for us to apply the patch.

The CreateDeviceAsyncEventSource? warning part of the patch is fine. I might have already rolled it in to my own tree.

[rogueresearch]

hjelmn, wrt to the current CFRunLoopGetCurrent code: I would argue that my patch is more correct. If you CFRelease() something too much, you crash; if you CFRetain() it too much, you leak; the balance must be perfect. The current code assumes that each invocation of CFRunLoopGetCurrent() returns the exact same pointer. That seems to be a safe assumption, but why even assume it? To me, it's more semantically correct to exactly balance CFRetain/CFRelease with the exact variable. It's more future-proof against changes to CFRunLoopGetCurrent()'s implementation, and to future changes to libusb.

[rogueresearch]

In [5e36dd39bf5ac3c98a780dd62813c79cfe0a4ed6/libusb-pbatard]:

io.c: Fix clang analyzer warning about unused variable

References #28.

[rogueresearch]

In [71b380b0b0046c9442ee3bdfd6e9cc6b1f8ba3aa/libusb-pbatard]:

Darwin: Fix #28 clang analyzer warning about unbalanced retain/release

[rogueresearch]

Confirmed fixed in ​http://git.libusb.org/libusb.git "testing" branch. Analyzer no longer warns about anything.

[rogueresearch]

In [b67120f047f7eafa15c88c66fa61cef40805ec1f/libusb]:

io.c: Fix clang analyzer warning about unused variable

References #28.

[rogueresearch]

In [3ba2fae24886fec89410e5f2295f65363edcc2df/libusb]:

Darwin: Fix #28 clang analyzer warning about unbalanced retain/release