On Thu, Jun 9, 2011 at 9:06 AM, Øyvind Harboe <oyvind.har...@zylin.com>wrote:
> On Thu, Jun 9, 2011 at 7:29 AM, Peter Stuge <pe...@stuge.se> wrote: > > Øyvind Harboe wrote: > >> Nit, the construct below is a little bit fancy. I'd prefer splitting > >> it over multiple lines to make it more accessible to the > >> casual reader. > That requires adding a lot of brackets. > > > Or just use goto, please, which is clear, simple, well understood, > > and elegant all at once. :) > > I think goto is OK sometimes. When it is used to unwind the stack > frame and free up resources, in lieu of exceptions and resource > tracking it can clean up the code significantly. > I'm all for gotos in error handling. In this simple case it wouldn't have made things cleaner and the patch would have grown. Also I didn't want to join the goto-vs-subfunction-flamewar-to-be, so a third option was convenient. :) Main objection is that people are not very used to the comma operator. I don't mind rewriting it using goto if the list feels that would be preferred. > > The patch uses code duplication and if statements inside the > cleanup fn(if (buffer) free(buffer)), which can be avoided using > exception handling and resource tracking. In the case of this patch, > it looks like the if (ft2232_buffer) free(ft2232_buffer) is a case > of "conservative programming". I'd much rather have an assert > in this case than to have broken code "auto-repair" and be harder > to get right. > Duplication? Rather code reuse. Ok, the repeated calls to the cleanup function (or the equivalent gotos, if you want to go that route) would have disappeared using exception handling. But C doesn't have it, so... The if statement was added to make ft2232_quit() a bit more generic so it could be used for all cleanup cases, including the error case where malloc fails. The alternative would have been to add a separate error handling path for that case. And given how often error paths are exercised, it's safer to keep them to a minimum. > Note that I'm not holding off committing the patch for the above, as > maintainer I'm interested in progress as well as more philosophical coding > style discussions. :-) > > If someone does come forth with an even more cleaner patch, then > I'm OK with that. This is now growing into a case-study of how to formulate > a patch more than a fix. The openocd community has much to learn > about how to formulate patches. > > As long as there is progress, I don't mind holding off the patch. > > I'm not in a hurry. There's a lot to do in the ft2232 driver. I have the intention to perform some general cleanup in the near future, will post some thoughts this evening maybe. /Andreas
_______________________________________________ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
