Skip to content

Keyword swap changes#1539

Merged
chenglou merged 2 commits into
reasonml:masterfrom
hcarty:keyword-swap-changes
Oct 24, 2017
Merged

Keyword swap changes#1539
chenglou merged 2 commits into
reasonml:masterfrom
hcarty:keyword-swap-changes

Conversation

@hcarty

@hcarty hcarty commented Oct 22, 2017

Copy link
Copy Markdown
Contributor

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

hcarty commented Oct 22, 2017

Copy link
Copy Markdown
Contributor Author

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

hcarty commented Oct 22, 2017

Copy link
Copy Markdown
Contributor Author

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
jordwalke reviewed Oct 22, 2017
View reviewed changes

@jordwalke jordwalke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/syntax_util.ml Outdated
in
(str, payload)

let string_prefix ~prefix s =

@jordwalke jordwalke Oct 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use a comment explaining what string_prefix does.

@jordwalke jordwalke Oct 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hcarty Oct 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hcarty Oct 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to review!

@hcarty hcarty force-pushed the keyword-swap-changes branch from ba3b281 to 7789f4d Compare October 23, 2017 04:05
@hcarty

hcarty commented Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

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
chenglou reviewed Oct 23, 2017
View reviewed changes
Comment thread src/syntax_util.ml
| x when (
potentially_conflicts_with ~keyword:"switch_" x
|| potentially_conflicts_with ~keyword:"pub_" x
|| potentially_conflicts_with ~keyword:"pri_" x) -> string_drop_suffix x

@chenglou chenglou Oct 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hcarty Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@chenglou

chenglou commented Oct 24, 2017

Copy link
Copy Markdown
Member

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

Thanks!

@chenglou chenglou merged commit 38ac8a5 into reasonml:master Oct 24, 2017
@hcarty hcarty deleted the keyword-swap-changes branch October 24, 2017 05:45
chenglou added a commit to chenglou/reason that referenced this pull request Oct 24, 2017
@chenglou
Enhance reasonml#1539
fc32d4e
chenglou added a commit that referenced this pull request Oct 24, 2017
@chenglou
Enhance #1539 (#1542)
7b13876
@jordwalke

jordwalke commented Oct 24, 2017

Copy link
Copy Markdown
Member

Thanks everyone!

@chenglou chenglou mentioned this pull request Oct 24, 2017
Closed
@stedolan stedolan mentioned this pull request Jul 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jordwalke jordwalke jordwalke left review comments
+1 more reviewer
@chenglou chenglou chenglou left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hcarty @chenglou @jordwalke @facebook-github-bot