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] Building OpenOCD for Windows
New package for Windows is available for download on my website www.freddiechopin.info 4\/3!! ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
[Openocd-development] openocd patch: 8cb99c4 dsp563xxx: fix missing error propagation found by clang
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/140
-- gerrit
commit 8cb99c45f508b20fbac9aa9649374b774cb647d8
Author: Øyvind Harboe oyvind.har...@zylin.com
Date: Sun Oct 30 18:36:47 2011 +0100
dsp563xxx: fix missing error propagation found by clang
Change-Id: I7380ce145b4942e21b174f2a810928a877c32bc7
Signed-off-by: Øyvind Harboe oyvind.har...@zylin.com
diff --git a/src/target/dsp563xx.c b/src/target/dsp563xx.c
index b7f23c7..6a59e86 100644
--- a/src/target/dsp563xx.c
+++ b/src/target/dsp563xx.c
@@ -1323,7 +1323,7 @@ static int dsp563xx_run_algorithm(struct target *target,
int timeout_ms, void *arch_info)
{
int i;
- int retvaltemp,retval = 0;
+ int retval = ERROR_OK;
struct dsp563xx_common *dsp563xx = target_to_dsp563xx(target);
if (target-state != TARGET_HALTED)
@@ -1376,9 +1376,13 @@ static int dsp563xx_run_algorithm(struct target *target,
for (i = 0; i num_mem_params; i++)
{
if (mem_params[i].direction != PARAM_OUT)
- if ((retvaltemp = target_read_buffer(target,
mem_params[i].address, mem_params[i].size, mem_params[i].value)) != ERROR_OK)
+ retval = target_read_buffer(target,
+ mem_params[i].address,
+ mem_params[i].size,
+ mem_params[i].value);
+ if (retval != ERROR_OK)
{
- retval = retvaltemp;
+ return retval;
}
}
--
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] Gerrit mail subject
openocd-development-boun...@lists.berlios.de wrote: Mathias K. wrote: can you remove the magic incomplete number too? In this case the 1533d9d. This number is useless in the subject: I disagree. This is an abbreviated commit hash, which identifies the commit that was pushed. There's also Gerrit's identifiers, but losing the commit hash would be impractical. Having the abbreviated hash in the email makes sense, but at the start of the subject is just noise and a waste of space, IMO. I like the [REVIEW] idea, and don't see any need for openocd to be a string in the subject at all. There are headers for sorting and the To: and From: make it pretty clear what project the patch is for. So [Openocd-development] New openocd patch: hash123 subsys: desc I would reduce to [REVIEW] subsys: desc Much faster to get what I need for an eyeball scan, which is what I use Subject: for. -- Jon Povey jon.po...@racelogic.co.uk Racelogic is a limited company registered in England. Registered number 2743719 . Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB . The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] Gerrit mail subject
Mathias K. wrote: can you remove the magic incomplete number too? In this case the 1533d9d. This number is useless in the subject: I disagree. This is an abbreviated commit hash, which identifies the commit that was pushed. There's also Gerrit's identifiers, but losing the commit hash would be impractical. Okay, is it possible to change the commit message on a commit with the same commit hash? No, besides contents the hash also depends on commit time and commit message. That's exactly why the hash is useful, to identify the particular commit that is refered to. //Peter ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
