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
