softdevteam / lrpar

Rust LR parser
Other
1 stars 0 forks source link

Use a dynamic notion of distance #42

Closed ltratt closed 6 years ago

ltratt commented 6 years ago

This PR consists of two initial refactoring commits (both very easy) before the "big one" (though it's not big in terms of code): moving to a dynamic notion of distance in the A* algorithm. The commit message for https://github.com/softdevteam/lrpar/commit/b42e1b4c0b3812173b775f22c630181ddb9c0d94 hopefully explains this in sufficient detail.

ptersilie commented 6 years ago

I'm not sure I can follow the explanation of the last commit. If U follows T how can the distance to U (1) be smaller than the distance to T (2)? Shouldn't the distance to U be the distance to T plus the distance to U?

ltratt commented 6 years ago

Good question. I didn't explain one important detail.

If the input is valid, then U's distance must always be > than T's. But if the input is invalid, the user might have put a silly T in that has a greater distance than U.

Imagine if I had a Java input something like var x = } 5;. I think I'm right in saying that the distance from = to } is 2, but the distance from = to 5 is 0.

ptersilie commented 6 years ago

Aaaaahhhhh! That explains everything. :)

ltratt commented 6 years ago

If it's any consolation, I first had this rough thought over 4 weeks ago, but it took me ages to work out how to trigger this in practise... It's really not obvious (at least... it wasn't to me!).

ptersilie commented 6 years ago

It's obvious when you explain it like this. But I can see how finding this example isn't obvious at all.

ptersilie commented 6 years ago

So just a few typos. Rest looks fine.

ltratt commented 6 years ago

OK, fixed both typos. Can squash if you'd like?

ltratt commented 6 years ago

Actually, maybe I should add that Java example in to the comment first...

ltratt commented 6 years ago

OK, example added. If you think that's good, I can squash.

ptersilie commented 6 years ago

Yes, I think the java example is really helpful for the understanding in that commit. I'm happy with this. Squash away and I will merge.

ltratt commented 6 years ago

Squashed.

ptersilie commented 6 years ago

Nice. Merged.