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)

[Øyvind Harboe]

 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)

[Andreas Fritiofson]

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