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
[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)
On Sat, Oct 29, 2011 at 10:56 AM, ger...@openocd.zylin.com wrote:
This is an automated email from Gerrit.
?yvind Harboe (oyvindhar...@gmail.com) just uploaded a new patch set to
Gerrit, which you can find at http://openocd.zylin.com/134
-- gerrit
commit 620ba7e6cd57c951fafa0f1ffab2db102fe2a60f
Author: Øyvind Harboe oyvind.har...@zylin.com
Date: Sat Oct 29 10:55:02 2011 +0200
clang: fix warning by adding assert that shows that a variable is used
Change-Id: I26e0ba9f1ae9d43c9a14c42c4225746782dc4d66
Signed-off-by: Øyvind Harboe oyvind.har...@zylin.com
diff --git a/src/jtag/tcl.c b/src/jtag/tcl.c
index 468edf5..b634ac0 100644
--- a/src/jtag/tcl.c
+++ b/src/jtag/tcl.c
@@ -166,6 +166,8 @@ static int Jim_Command_drscan(Jim_Interp *interp, int
argc, Jim_Obj *const *args
}
} /* validate args */
+ assert(e == JIM_OK);
+
tap = jtag_tap_by_jim_obj(interp, args[1]);
if (tap == NULL) {
return JIM_ERR;
I'm not very fond of the idea of merging patches with the sole purpose of
fixing scan-build false positives. clang is not perfect and 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.
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).
The correct fix would be to reduce the scope of 'e' to inside the loop,
which would _increase_ code quality as well as (probably) silence
scan-build. My point being that silencing a warning must only ever be a
side effect of increasing code quality. The warning _may_ be indicative of
a real bug, and forcing the warning to disappear willy-nilly also makes the
bug a lot harder to find for someone that uses scan-build to try
to pro-actively improve the code base.
Of course, I would have added this comment in gerrit, if the patch hadn't
already been merged, a measly few hours after upload, by the author
himself, without comments from anyone else.
No point in having a fancy review system if we're not given the chance to
review. Please allow patches to remain in the queue for *days* not *hours*.
Gerrit guarantees that no patch will be forgotten, so what's the hurry? I
have no possibility to review, or at least comment on, patches during
work-hours and sometimes not until the weekends. I'm sure I'm not alone. By
then, the patches left in the queue are mostly those that have big red X's
or the work-in-progress ones. And to my fellow maintainers; be extra
patient with patches you've written yourself and wait for at least some
form of feedback before merging. It's not that easy to spot flaws in one's
own code so it's always good to get another pair of eyes on it, even if it
_should_ be trivial.
Regards,
Andreas
___
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
