combine foldConstants with standardizeTree

[mr-martian]

I made stadardizeTree() do the job of foldConstants(), since there's really no need to recurse through the whole tree twice. I also made it fold binary nodes (e.g. 5*5 -> 25, "hi" === 3 -> false). If you don't want the added functionality, just comment out the if statement in lines 91-114.

As far as I can tell, this has resulted in a significant performance increase, from around 480 ms to run all the tests to approximately 390 ms. (alternatively, I could state this as: it's roughly back to where it was before I started messing with it [380 ms], though this does not take into account the impact of adding 22 more tests)

[mr-martian]

Leaving it uncommented will result in the failure of test 4 of section 12 (Multiple multiple-var callbacks work.).
This is due to trying to match _ += $a + $b; against tree += 30 + 50 + 10;, after constant folding, this results in tree += 90, which of course, does not match.

[k4b7]

@mr-martian cool stuff. I will try to review this this weekend.

[k4b7]

@mr-martian I've only been able to have cursor look at your changes. With regards to the constant folding, could you pull those changes out for now. If we do decide to add constant folding I'd prefer to have it in a separate commit behind an option with tests.

I'm trying to understand in when constant folding would be helpful. Do you have some examples?

[mr-martian]

I've removed the folding entirely.
I imagine it would be helpful in a situation where you were looking for x + 17 and the user for whatever reason entered x + 10 + 7

[k4b7]

I thought that lines 91-114 included new functionality that you were experimenting with. If I understand it correctly, those lines implement the same functionality that was in foldConstants plus you've added support for additional binary operators beyond just + and -.

> This is due to trying to match _ += $a + $b; against tree += 30 + 50 + 10;, after constant folding, this results in tree += 90, which of course, does not match.

This gave me the impress that constant folding itself was causing test failures, but since foldConstants was in the code before these changes and those tests were passing, it isn't constant folding, but rather the way it's implemented now.

If that's a fair assessment of the situation can you add the code back in and uncomment those lines and let's try to figure out what's causing those failures.

[k4b7]

Looking at the code for foldConstants I see that it was only folding unary expressions. Yeah, let's keep the binary expressions out. I'd like to be able to run those changes against the regression tests that we have for the challenges.

[mr-martian]

Sorry if I was unclear, the functionality that was foldConstants is now the case "UnaryExpression": just below the commented lines. The commented (now removed) lines were only folding constants in binary and logical expressions.

[k4b7]

@mr-martian sorry for letting this stall. I want to run this against the integration tests for all of our challenges. Unfortunately, these tests aren't automated so it'll take some time. I'll have some time in a couple of weeks to do that.