Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scope hack for compiler #55

Merged
merged 2 commits into from
Jul 19, 2017

Conversation

haya14busa
Copy link
Member

This change allow us to translate vimlparser.vim to
other language (e.g. Go) which creates scope in "if" block.

@mattn
Copy link
Member

mattn commented Jul 19, 2017

I want to to hear another one's opinion. @blueyed @Kuniwak any thought?

@blueyed
Copy link
Contributor

blueyed commented Jul 19, 2017

Cannot say anything about it, sorry.

return node
endif
Copy link
Member

Choose a reason for hiding this comment

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

add indent space into previous line?

@mattn
Copy link
Member

mattn commented Jul 19, 2017

@haya14busa Ah, I just understand what you want to fix.

@@ -2987,9 +2990,9 @@ ExprParser.prototype.parse_expr8 = function() {
if (token.type != TOKEN_SQCLOSE) {
throw Err(viml_printf("unexpected token: %s", token.value), token.pos);
}
var left = node;
Copy link
Member

Choose a reason for hiding this comment

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

Is this your expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes? Maybe I just don't understand what you asked?

It's exactly translated as I expected.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, you don't expect var here, I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

It works because it's "var", not "let".

Ideally, we may want to drop "var" here but it's not this p-r scope.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. hopefully we will fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

VIm script's "let" ≒ JavaScript "var"

Cool! Easy to translate! Happy variable scope life! 😇 😇 😇

@thinca
Copy link
Member

thinca commented Jul 19, 2017

I think that this can not be maintained continuously.
This should be solved in translater program.

This change allow us to translate vimlparser.vim to
other language (e.g. Go) which creates scope in "if" block.
@haya14busa
Copy link
Member Author

Hmm... to be honest... I agree with you.

This change is the most hackly one to create go-vimlparser and it's indeed hard to maintain.
I think it's also true that we can solve it in translater program.
It's going to be difficult work though...

@mattn
Copy link
Member

mattn commented Jul 19, 2017

As I mentioned in this issue, #61 (comment)

We can't fix this problem, I guess. To fix this issue, we must change let as below

current

var s = 1

newer

this.s = 1

i.e. We can't use var since prepending var changes behavior of exists('s') .

@haya14busa
Copy link
Member Author

Hmmm... maybe there is miscommunication.
There are no problem with current vimlparser including python/javascript version with this p-r except maintainability problem. (JavaScript tests are passed.)

If you talk about this problem as general vimltranslator, yes, we have to care about "exists()", but I think as far as the translater in this repository is for translating autoload/vimlparser.vim, we should not use "exist()" in the first place.
(If you take a look at jscompiler.vim or pycompiler.vim, there are lots of workaround.)

It's indeed a problem if you want to create general Vim script (to JavaScript) translators, but it's not so high priority problem if you don't have plan to create it.

@mattn
Copy link
Member

mattn commented Jul 19, 2017

@haya14busa I don't think your changes in this PR still have some problem. I just want to say that we can't make perfect compiler since specification of Vim scripts have ambiguous for that let have two meanings let and define. So I'll merge this.

@mattn mattn merged commit 55f97af into vim-jp:master Jul 19, 2017
@haya14busa
Copy link
Member Author

Oh, thank you for merging this.

I was about to close this p-r and solve the problem by go-vimlparser side.

Let's revert this p-r if there are maintainability problem. Feel free to revert it.

Hopefully we can enjoy benefit, called "static type-check" by adding gocompiler.vim into this repository soom by including this p-r!

@haya14busa haya14busa deleted the add-scope-hack-for-compiler branch July 19, 2017 15:06
@mattn
Copy link
Member

mattn commented Jul 19, 2017

Okay, I agree with your suggestion. looking forward your p-r. Or you will develop go version separated?

@haya14busa haya14busa mentioned this pull request Jul 19, 2017
haya14busa added a commit to haya14busa/vim-vimlparser that referenced this pull request Jul 19, 2017
Follow up of vim-jp#55

I forgot to change this function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants