Operator precedence

Jul 11, 2013 at 5:57 PM

I'm having trouble with operator precedence. Observe the following grammar:
            // 1. Terminals
            var number = TerminalFactory.CreateCSharpNumber("number");
            number.AstConfig.NodeType = typeof(NumberNode);

            // 2. Non-terminals
            var expr = new NonTerminal("Expression", typeof(ExpressionNode));
            var binOp = new NonTerminal("BinOp", typeof(BinOpNode));
            var binExpr = new NonTerminal("BinExpr", typeof(BinExprNode));

            // 3. BNF rules
            expr.Rule = number | binExpr | "(" + expr + ")";
            binExpr.Rule = expr + binOp + expr;

            KeyTerm plus = ToTerm("+");
            KeyTerm minus = ToTerm("-");
            KeyTerm mult = ToTerm("*");
            KeyTerm div = ToTerm("/");
            binOp.Rule = plus | minus | mult | div;

            // 5. Operators precedence
            RegisterOperators(1, plus, minus);
            RegisterOperators(2, mult, div);

            // 6. Miscellaneous: punctuation, braces, transient nodes
            MarkPunctuation("(", ")", ":");
            RegisterBracePair("(", ")");

            this.Root = expr;
            this.LanguageFlags = LanguageFlags.NewLineBeforeEOF | LanguageFlags.CreateAst;
If I use this grammar to parse the expression:

3 * 4 + 5

I'd assume that I get parse tree with the binary expression with + as the root of the tree, because * has higher precedence than +. However, that is not the case. I actually get a parse tree with the * as the root expression.

Have I misunderstood how precedence work in Irony, or is something wrong?

Best regards,
Per Rasmussen
Jul 12, 2013 at 5:54 AM
Edited Jul 12, 2013 at 5:55 AM
ha... really??... yep... a bug..
to make it work, just add this:
  this.MarkTransient(binOp, expr);
without it, when operators are hidden under extra binOp node, parser fails to find precedence values.. but with this line, parse tree looks better anyway with transients removed
wil fix it - thanks and sorry for trouble
Jul 12, 2013 at 5:56 AM
PS Please create an issue (on Issues page), so I won't forget
Jul 12, 2013 at 8:10 PM
Hi Roman. Thank you very much. I've created an issue for this bug. Perhaps I could take a crack at fixing it. In that case I have two questions:
  1. If I write a fix for this, can I simply commit the changes, or how does that work?
  2. Can you provide a hint or two about where in the code I should be looking and what I'm looking for?
Jul 12, 2013 at 8:21 PM
no problem. don't worry about fixing it, the code is messy there, it used to work but I removed the case that handled it properly in one of the refactorings. I would be simpler for me to fix it myself
Jul 15, 2013 at 9:22 PM
Yes, I have noticed that there are parts of the codebase that aren't too pretty (no offense, bro) :)

But perhaps I could contribute in some other way. For example, I've been thinking about making a new way to create an AST. I'm not really a fan of the current solution, but I have a few ideas.

Jul 17, 2013 at 6:26 PM
go ahead and work out smth if you know you can do something better. The standard way is to clone the repo, and then submit a pull request. Alternatively, if your code is just a bunch of extra classes in a separate folder, you can just zip and email it to me
Feb 9, 2014 at 8:39 AM
I'm having the same trouble. Marking nodes as transient solves it, but I still waiting for fix :)