[asterisk-dev] [Code Review] Various fixes for extenpatternmatchnew

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/194/#review709
-----------------------------------------------------------

Ship it!



/trunk/main/pbx.c
<http://reviewboard.digium.com/r/194/#comment1840>

    I really don't like this huge macro that is defined in the middle of a function.  It really should just be a function.  But, this isn't new code, so I guess we can change it later.



/trunk/main/pbx.c
<http://reviewboard.digium.com/r/194/#comment1842>

    Same comment goes for this macro.  It's a huge macro defined in the middle of a function that really should just be a function.
    
    If you feel like making a function out of it, that would make me happy, but it's not required since it's not new code.



/trunk/main/pbx.c
<http://reviewboard.digium.com/r/194/#comment1841>

    lost a space on this line


- Russell


On 2009-03-13 15:35:46, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/194/
> -----------------------------------------------------------
> 
> (Updated 2009-03-13 15:35:46)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When looking into issue 14615, I found a variety of problems which I have addressed in my npm_fixes branch. Here is a summary of the fixes I made:
> 
> 1. Create different trie entries for pattern and non-pattern matches which are otherwise identical. For example, if you had an extensions "NNN" and "_NNN" in your dialplan, then we need to create two separate trie entries for matching against.
> 
> 2. Make sure to only apply pattern-matching logic if the extension we are matching against is a pattern extension. This is what bug 14615 is actually about. In that example, the reporter was attempting to Goto() the extension "ANSWER." The problem was that when the 'N' was encountered while looking at the pattern trie, it would not match the literal character 'N' that was being passed in. However, if the Goto() were modified to go to something like "A4SWER" it would go to the ANSWER extension. This behavior has been corrected.
> 
> 3. Fixed a few places where the code could potentially crash due to dereferencing NULL pointers.
> 
> 4. Changed the allocation scheme of a match_char to use the "allocation after the struct" method.
> 
> 5. Did a bunch of coding guidelines-related cleanup.
> 
> 6. Removed all blocks of code enclosed inside #ifdef NOTNOW...#endif. These areas were designed to use a more manual algorithm for looking up contexts than just finding them in a hashtab. The logic used in these blocks was off, and I can't think of any good reason why you would want to do this.
> 
> For anyone who wants to understand how the pattern-matching trie works before taking a crack at this review, you can find a large comment in main/pbx.c that Murf wrote that does an excellent job of explaining things. This comment block is located just below the function pbx_destroy().
> 
> 
> This addresses bug 14615.
>     http://bugs.digium.com/view.php?id=14615
> 
> 
> Diffs
> -----
> 
>   /trunk/main/pbx.c 182115 
> 
> Diff: http://reviewboard.digium.com/r/194/diff
> 
> 
> Testing
> -------
> 
> I tested the following dialplan, admittedly not the most thorough, but it gets the job done:
> 
> exten => 333,1,NoOp(Starting Test, exten 333)
> exten => 333,n,Goto(ANSWER,1)
> exten => 333,n,Hangup
> 
> exten => NNN,1,NoOp(Starting Test, exten NNN)
> exten => NNN,n,Playback(tt-monkeys)
> exten => NNN,n,Hangup
> 
> exten => _NNN,1,NoOp(Starting Test, exten _NNN)
> exten => _NNN,n,Playback(tt-weasels)
> exten => _NNN,n,Hangup
> 
> exten => ANSWER,1,NoOp(Test no answer)
> exten => ANSWER,n,Playback(vm-goodbye)
> exten => ANSWER,n,Hangup
> 
> exten => _1[2-9],1,NoOp(Starting Test, exten _1[2-9])
> exten => _1[2-9],n,Playback(queue-thankyou)
> exten => _1[2-9],n,Hangup
> 
> With this, I could test numeric extensions, string extensions, patterns with reserved letters (i.e. N, Z, X), and patterns with bracketed expressions. In all cases, the pattern I expected to match was matched. The reporter on issue 14615 has also given the branch a test and says that things work for him in his dialplan now.
> 
> 
> Thanks,
> 
> Mark
> 
>