Keyword swap changes

[hcarty]

Fixes #1361 -- with the exception that it doesn't update any tests yet and has received minimal testing aside from some quick checks:

$ echo 'let pub = 5' | ./refmt_impl.native --parse=ml --print=re
let pub_ = 5;
$ echo 'let pub_ = 5' | ./refmt_impl.native --parse=re --print=ml
let pub = 5

[hcarty]

Does this seems like a reasonable approach? I don't have node setup locally so I can't do much in the way of thorough testing until I fix that as it looks like make test requires node now.

It would be REALLY nice to have this or an equivalent in before the big release, if that's at all possible.

[hcarty]

And there are bad bugs in the string_prefix function which I'll fix as soon as I'm in front of a computer again.

[jordwalke]

Thanks for doing this, @hcarty. I agree this would be a good change to get in for the release. I had a question about the change.

[jordwalke]

This could use a comment explaining what string_prefix does.

[jordwalke]

And could it ever be the case that s is smaller in length than prefix (causing runtime error)? For example, an identifier such as "matc". Do you want to do something like:

let matched = ref true in
   let n = String.length prefix in
   if String.length s >= n then
     for i = 0 to n - 1 do
       matched := !matched && prefix.[i] = s.[i]
     done;
     !matched
   else
     false

[hcarty]

Yeah - sorry about that! That's the bug I left a comment about. I realized I hadn't handled that case just after I turned off my computer.

This could also short-circuit by raising and immediately catching an exception when a character doesn't match.

I'll fix these issues and add some comments when I'm in front of a computer again, hopefully tomorrow.

[hcarty]

Thanks for taking the time to review!

[hcarty]

Bug fixes, better function names, more comments, short-circuiting and rebased. This should be in a much better state now.

As @jordwalke noted, the previous version would fail badly if the identifier was shorter than the prefix being tested against. That should be fixed now.

A more subtle but still quite bad bug was that the _ suffix meddling was applied for any identifier starting with a keyword. So both pub and publish would have been modified when translating between syntaxes even though publish does not have a conflict. This has been fixed so that only keywords with zero or more _ trailing characters are affected by the name substitution.

[chenglou]

This is doing quite a few checks. Shouldn't it be calculated once then have the result pattern-matched on, in a nested match?

[hcarty]

There are a lot of checks there. What do you mean by calculated once?

[chenglou]

Tested and works. I'll put up some more comments and better tests. The current ones aren't indicative anymore.

Thanks!

[jordwalke]

Thanks everyone!