Re: [Openocd-development] scan-build and gerrit rant
if we start tweaking perfectly good code and adding nonsense checks just to get a clean scan-build output, I think that's a step backwards in terms of code quality. Yes, I agree. I'm not at all fond of throwing assert() at the clang warnings. +1 from me aswell. Spen ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] scan-build and gerrit rant
There are a couple of topics mixed into this thread: 1. clang sometimes gives false positives. Here the code is usually so complex that I can understand that clang and programmers have trouble following the code. The best way would be to refactor the code to be simpler, in one case I split a long and complicated fn instead of using an assert. I'm not saying that clang can't give silly false positives, but when it does give false positives, it's oftentimes in too-long functions. So far I have yet to see clang give silly warnings on highly readable code. 2. the gerrit review process and build system is new, so I've been using simple warning fixes to take it for a spin. 3. w.r.t. the review process, I was thinking if we should have a rule like: a patch can be submitted if a week has gone without feedback and it looks good, or a second reviewer approves it. -- Øyvind Harboe - Can Zylin Consulting help on your project? US toll free 1-866-980-3434 http://www.zylin.com/ ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] scan-build and gerrit rant (Was: Re: openocd patch: 620ba7e clang: fix warning by adding assert that shows that a variable is used)
In this case, the warning is probably bogus (I'll have to check the scan-build output but having problems logging in to jenkins). Unfortunately, the fix is, too. There's no point in adding an assert to check for the value of a variable when that value has no possible bearing on the program (the variable is never used after the assert, which, incidentally, was exactly what scan-build complained about). I think the correct fix here is to split the fn. The assert() I added amounts to a post-condition of the fn I want to split out. To split out the fn, I need a bigger monitor than I have now, or a re-factoring tool. assert()'s are designed to be used for pre and post conditions. OpenOCD certainly does not suffer from over-usage of asserts. If we ignore what clang is complaining about, then I think we can agree that clang found a function that is too big and complex. Once this fn has been split in two, then copy and paste code can be avoided more easily for ir/drscan versions of the same command. clang is new to me, so I'm still learning to interpret it's warnings into sensible refactorings. -- Øyvind Harboe - Can Zylin Consulting help on your project? US toll free 1-866-980-3434 http://www.zylin.com/ ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] scan-build and gerrit rant
Andreas Fritiofson wrote: clang: fix warning by adding assert that shows that a variable is used .. + assert(e == JIM_OK); + .. I'm not very fond of the idea of merging patches with the sole purpose of fixing scan-build false positives. +1 if we start tweaking perfectly good code and adding nonsense checks just to get a clean scan-build output, I think that's a step backwards in terms of code quality. Yes, I agree. I'm not at all fond of throwing assert() at the clang warnings. Thanks //Peter ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
