On Fri, 2007-11-09 at 09:10 -0800, Luigi Rizzo wrote: > On Fri, Nov 09, 2007 at 04:00:23PM -0000, SVN commits to the Asterisk project > wrote: > > Author: murf > > Date: Fri Nov 9 10:00:22 2007 > > New Revision: 89129 > > > > URL: http://svn.digium.com/view/asterisk?view=rev&rev=89129 > > Log: > > This is the perhaps the biggest, boldest, most daring change ... > > I have no objection with this specific change and the features it > adds/modifies, but I am very concerned with the way this is being brought in. > > This (and a number of other asterisk source files) are already > longer than reasonable. Adding the entire matching code inline, > instead of using a separate file, makes the code even worse from > the point of view of maintaining it. > > What you should have done is: > - define a suitable api for extension matching (functions to be called, > perhaps extra arguments in the ast_context); > - add the extra arguments as void * to ast_context; > - define global function pointers for the various matching functions, > assigning them by default to the current implementation; > - put the new extension matching code in a new file, e.g. pbx_match2.c, > which is linked with the existing pbx.c code; > - add a config file option (and perhaps cli method) to select which > method you want to use at runtime. > > This is not a lot of work (i did the same when i introduced the new > "say" functions), especially because at a quick look your code seems > well documented and structured, so it lends well to the modifications > i am suggesting. > What you gain is that the code becomes a lot more manageable, and > it is a lot less risky for people to test trunk, because there is > a trivial way to switch back and forth between the two implementations > and compare behaviour. > > I hope you will follow this suggestion.
Excellent advise-- A reasonable request... I have it at the top of my list. murf
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ --Bandwidth and Colocation Provided by http://www.api-digital.com-- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
