grom358 / pharborist

A PHP library to query and transform source code via tree operations.
GNU General Public License v3.0
44 stars 10 forks source link

Condense binary operation nodes into a single node type #183

Closed phenaproxima closed 10 years ago

phenaproxima commented 10 years ago

This came up on IRC at some point, but I think it'd be awesome to simply have a few node types for binary operations instead of a node type for each operator. This would give us the freedom to change the operation easily, too.

We should have one class, BinaryOperationNode, which covers the =, ==, ===, !=, !==, <, <=, >, >, and instanceof operators (I hope I haven't forgotten any). We can have getOperator() and setOperator() methods. IMO this would be cleaner than what we've currently got. We could also have isComparison(), isBitwise(), isAssignment(), isInstanceOf(), etc.

(Another possibility is to have a base BinaryOperationNode class, then a subclass for comparisons (ComparisonNode), bitwise operations (BitwiseOperationNode), assignment (AssignNode, unchanged), and instanceof (presumably we already have an InstanceOfNode)).

grom358 commented 10 years ago

I would have to vote for your second option there. Add more to class hierarchy for the operators.

The reasons for having a class per operator are:

Do you have example use case of where you wish to change the operator? There is no simple way of doing this, you can't just change the operator being used due to operator precedence.

For example say you have:

$a = 4 + 2 + 3; // the tree is (= $a (+ (+ 4 2) 3))

And you change the 2 + 3 to 2 * 3 then the tree is wrong due to operator precedence. As it should be (= $a (+ 4 (* 2 3)))

pako-pl commented 10 years ago

Having a separate class for each operator makes static analisys a lot easier.

phenaproxima commented 10 years ago

Consensus is that this is not a good idea, and operator precedence makes it unfeasible. To the pit with this issue!