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 yarp/yarp_compiler.c #8042

Merged
merged 2 commits into from Aug 28, 2023
Merged

Conversation

jemmaissroff
Copy link
Contributor

Add structure code necessary for compiling YARP. I will continue to fill in the actual compilation code. This is just the stencil of how we'll implement RubyVM::InstructionSequence.compile_yarp

@tenderlove
Copy link
Member

LGTM. @ko1 do you think it's OK for us to add compile_yarp to RubyVM::InstructionSequence? Or should we add a :yarp option to compile? Or something else?

@ko1
Copy link
Contributor

ko1 commented Jul 12, 2023

2 points:

  • the name yarp is not stable I guess so just now I'm against about introducing yarp related name.
    • I understand the name of yarp is a code name. I think the yarp library should be renamed too.
    • at least ask matz about the naming. If matz says okay, i have no problem on that.
  • I think YARP::compile... is preferable because we can separate the implementation from libruby.
    • it means that yarp related code is in yarp.so and libruby can use it just after requiring the yarp.so.
    • of course, the name YARP should be renamaed (or ask Matz) because of the previous poiint.
    • another reason is that if YARP replaces parse.y, the name compile_yarp doesn't make sense (or simply verbose) so I'm against to introduce it.

@tenderlove
Copy link
Member

@ko1 I think renaming isn't a big deal, we just need to have a better name. Any suggestions? 😅

@jemmaissroff jemmaissroff force-pushed the iseq-after-mirror branch 3 times, most recently from 5284899 to 1e3f31c Compare July 14, 2023 14:44
@ko1
Copy link
Contributor

ko1 commented Jul 14, 2023

Please ask matz!

@eregon
Copy link
Member

eregon commented Jul 18, 2023

I replied on https://bugs.ruby-lang.org/issues/19772.
In short:

  • I don't think it makes sense to rename YARP, IMO it's too late for that.
  • RubyVM::InstructionSequence.compile(yarp: true) seems best
  • YARP.compile is not OK, it would force to partially define the YARP module, and the implementation should be in compile.c AFAIK (because it depends on the specific bytecodes in a given CRuby version/commit, the YARP gem can't maintain that logic for multiple Ruby versions + this stuff only makes sense when part of CRuby (cannot be compiled outside of CRuby))

This commit adds yarp/yarp_compiler.c, and changes the sync script
to ensure that yarp/yarp_compiler.c will not get overwritten
@jemmaissroff jemmaissroff force-pushed the iseq-after-mirror branch 2 times, most recently from b400064 to 2137137 Compare August 28, 2023 17:29
…yarp

This commit creates the stencil for a compile_yarp function, which
we will continue to fill out. It allows us to check the output
of compiled YARP code against compiled code without using YARP.
@jemmaissroff jemmaissroff merged commit 3b815ed into ruby:master Aug 28, 2023
92 checks passed
@jemmaissroff jemmaissroff deleted the iseq-after-mirror branch August 28, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants