[asterisk-dev] [Code Review] Add support in AEL for macro return values and direct assignment of them to variables and functions.

[Steve Murphy]

> On 2008-12-31 15:39:06, Steve Murphy wrote:
> > Hmmmm. I see that you are doing substitutions in place when you see a macro call -- even when in an expression;
> > I don't think that's going to work very well in more than the simplest cases;
> > 
> > fred = &macro-with-return(5);
> > 
> > =>
> > 
> > Gosub(macro-with-return,s,1,...)
> > Set(fred=${GOSUB_RETVAL})
> > 
> > I just built with the patch and expanded your test case
> > a little, and I don't think this approach will fly 
> > very well.
> > 
> > macro test-retval (val1, val2) {
> >         local i=5+${val1};
> >         i=${i}+10*${val2};
> >         return ${i};
> > }
> > 
> > macro test-macro-assignment () {
> >         foo=40 * &test-retval(2,3);
> >         local bub=&test-retval(3,4);
> >         NoOp(foo is ${foo} and bub is ${bub});
> >         HASH(foo,bar)=&test-retval(9,7)+&test_retval(10,4);
> >         return;
> > }
> > 
> > And I'm getting syntax errors on line 11, the one with HASH().
> > 
> > In general, it's bad to try to do anything inside an expression in ael.y. It was everything I could do to make the scanner just collect it all and pass it in as a 'word'.
> > 
> > I think it'd be better to leave the
> > ael parser alone, and make the expr parser handle that
> > kind of logic in expressions... that way, we still
> > pack up the RHS and wrap it in $[] as we have;
> > 
> > fred = &macro1(x,y) + &macro2(z); 
> > =>
> > Set(fred,$[&macro1(x,y) + &macro2(z)])
> > 
> > Right now, if we try to do this above the expression level, you are going to have problems with calling
> > multiple macros, having them munge each others return
> > values, or limiting the syntax way more than is needful.
> > 
> > If you do this in main/ast_expr2.{y,fl}, then you have the ability to call the macro (somehow), and collect the 
> > result, atomically, and finish evaluating the expression
> > with no such problems.
> > 
> > In ast_expr, we already allow you to call asterisk functions without wrapping them in ${}:
> > 
> > $[ ${z} + MATH(..) ]
> > 
> > So, adding a macro shouldn't be impossible.
> > 
> > $[ &test-retval(9,7)+&test_retval(10,4) ]
> > 
> > should be parseable and evaluatable.
> > 
> > So, I propose that you might think along this line, and see what ael.y and ael.flex might do to allow this notation in exprs...
> >

No, no! I retract the above after discussing this with Tilghman. Yes, we could call out a new PBX to 
execute the gosub and return the result. But if the called gosub contained a call to a gosub (or recurses, or whatever),
we end with the EXACT same problem we had with Macros: 8 levels deep would be the limit before a crash.

So, rather than go there, Using a severely restricted grammar like what you implemented might actually not
be a bad compromise. Within the dialplan itself, gosubs have no recursion limit. 

Sorry to worry you with all this, but this is the first time I've had a chance to sit down and think
about the possibilities here. I'll continue the review come Monday.


- Steve


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


On 2008-12-30 17:59:32, Marquis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/114/
> -----------------------------------------------------------
> 
> (Updated 2008-12-30 17:59:32)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Add support in AEL for macro return values and direct assignment of them to variables and functions.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 166832 
>   /trunk/include/asterisk/ael_structs.h 166832 
>   /trunk/include/asterisk/pval.h 166832 
>   /trunk/pbx/ael/ael-test/ael-vtest26/extensions.ael PRE-CREATION 
>   /trunk/pbx/ael/ael-test/ref.ael-vtest26 PRE-CREATION 
>   /trunk/res/ael/ael.flex 166863 
>   /trunk/res/ael/ael.y 166863 
>   /trunk/res/ael/pval.c 166863 
> 
> Diff: http://reviewboard.digium.com/r/114/diff
> 
> 
> Testing
> -------
> 
> Ran the AEL regression tests in pbx/ael/ael-test.  Also adapted much of my dialplan (used in general PBXs all over the world) to use the new assignment.
> 
> 
> Thanks,
> 
> Marquis
> 
>