Skip to content

Conversation

@eregon
Copy link
Member

@eregon eregon commented Apr 7, 2019

… on MRI

* See [Bug #15743].
* Use "CRuby VM" instead of "Ruby VM" for clarity.
@eregon eregon changed the title Make it clear as possible that RubyVM is MRI-specific and only exists on MRI Make it as clear as possible that RubyVM is MRI-specific and only exists on MRI Apr 7, 2019
@eregon eregon requested review from ko1 and mame April 7, 2019 13:40
@k0kubun
Copy link
Member

k0kubun commented Apr 7, 2019

Why are you using both "MRI" and "CRuby"? How do you distinguish their usages?

@k0kubun
Copy link
Member

k0kubun commented Apr 7, 2019

Ah, "Ruby VM" -> "CRuby VM", but "MRI" by default. Maybe I understood.

@k0kubun
Copy link
Member

k0kubun commented Apr 7, 2019

I've heard of "MRI", "CRuby", and "Ruby VM", but I think I've never seen "CRuby VM".

While adding "MRI specific" descriptions looks good, I don't think it's a good idea to introduce a new confusing terminology "CRuby VM" to our source code as long as the similar terminology "Ruby VM" exists as a constant name RubyVM.

@eregon
Copy link
Member Author

eregon commented Apr 7, 2019

@k0kubun I used MRI for MRI specific because that's what existing documentation used.

Ruby VM is too ambiguous, I don't want to leave such ambiguity in the documentation (see https://bugs.ruby-lang.org/issues/15743 for more rationale).

I don't think it's a good idea to introduce a new confusing terminology "CRuby VM"

OK, let's try to find something else.
Why do you think it's confusing though?

Maybe for RubyVM::InstructionSequence, we should just use YARV since these are YARV bytecodes?
For other usages of CRuby VM, I can probably find other ways to say it.

@eregon
Copy link
Member Author

eregon commented Apr 7, 2019

@k0kubun I pushed 2 commits to avoid "CRuby VM", can you take a look? (I'll squash the commits when committing of course).


/*!
* Initializes the Ruby VM and builtin libraries.
* Initializes the VM and builtin libraries.
Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped Ruby because it's clear from the function name, and just VM is used in other cases as well such as ruby_finalize and ruby_cleanup.

@k0kubun
Copy link
Member

k0kubun commented Apr 7, 2019

Why do you think it's confusing though?

The only concern I have is that the similar terminology "Ruby VM" exists.

So I would be completely fine with the original change b5e6a1d if RubyVM were renamed to CRubyVM and the old constant name RubyVM were removed after some deprecation period.

let's try to find something else.

TBH I don't have a strong preference on the naming of RubyVM constant itself. What I don't like is just inconsistency in terminology usages.

For describing "Ruby VM", a7111cb looks better.

I pushed 2 commits to avoid "CRuby VM", can you take a look?

In that sense, the current pull request (at bacf54d) looks good to me. Thank you for your attention to my comment :)

@k0kubun k0kubun changed the base branch from trunk to master August 15, 2019 17:18
@k0kubun
Copy link
Member

k0kubun commented Aug 17, 2019

It seems to have a conflict now. Could you rebase this from master?

@k0kubun
Copy link
Member

k0kubun commented Aug 19, 2019

I fixed the conflict. Let's the current one and improve it afterwards as needed.

@k0kubun k0kubun merged commit 39a43d9 into ruby:master Aug 19, 2019
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.

2 participants