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

[Luigi Rizzo]

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.

	cheers
	luigi