[asterisk-dev] [asterisk-commits] murf: trunk r89129 - in /trunk: include/asterisk/ main/ pbx/ res/ael/ utils/

[Steve Murphy]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3239 bytes
Desc: not available
Url : http://lists.digium.com/pipermail/asterisk-dev/attachments/20071109/db00974a/attachment.bin