Øyvind Harboe wrote: > >> >> + if (detected_failure != ERROR_OK) > >> >> + return detected_failure; > >> >> + return retval; > >> > > >> > Please just use retval also for the error. .. > > the value of retval becomes irrelevant .. > Reading over the code, I can't match up what you write.
Please look again at the above. There is always only *one* return value from every function, so it seems logical that only one variable would be needed to store the value that the function will ultimately return. The code above from your patch highlights this too. The if() condition has a priority higher than the previous value of retval. If the condition is true, retval is irrelevant. Another way to express this is to remove the extra variable and the extra condition above and simply set retval (thus clobbering the now-irrelevant value of retval) at the point where detected_failure gets set. There are numerous benefits with regard to readability of the code. Most importantly the high level logic for this function stays more clear that it will be with two sources of return values being arbitered between at the *end* of the function, as opposed to a single source of (ie. single variable for) return value which in the normal case is not touched at all, but in error cases will be changed (since it is not irrelevant) into an error return code. > The normal behavior is: > > - detect and report failure. This is a failure that has occurred > in the hardware previously, there is no error condition currently. > The current function executes without any error as such. > > - once this error is detected, continue with the rest of the polling > code. > > If an error was detected(which may have occurred millions of cycles > before the current fn was even invoked), then report that error > at the end of the fn. detected_failure is *outside* the function? Sorry then, I guess I looked too quickly at the patch. :\ I still think the above is kinda ugly though. Is there any way for a nicer approach? Also, is the logic you just described really OK? It seems that some information about the target is getting lost? (The fact that the error did not happen *now* but some time ago, and that the current poll was OK.) //Peter _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
