fix: pass module name while evaluating cpp modules #327
Conversation
Samy-33
force-pushed
the
fix/module-name-while-loading
branch
from
dd01afa to
3b3e363
Compare
There was a problem hiding this comment.
Thanks for the PR, Saket. Aside from minor notes, I don't understand the reasoning of the primary change. If you've considered all of the questions I posed, please just explain with a comment in the code. Otherwise, please consider my questions and revise accordingly.
| @@ -1,5 +1,6 @@ | |||
| #pragma once | |||
|
|
|||
| #include "jtl/immutable_string.hpp" | |||
There was a problem hiding this comment.
It's highly unlikely this include is needed. However, if it is, it needs to use <> and needs to come after the third part includes, but before jank compiler/runtime includes.
| @@ -1,3 +1,4 @@ | |||
| #include "jtl/immutable_string.hpp" | |||
There was a problem hiding this comment.
This is definitely not needed, since you added it to the header. I don't think it was needed there either, though. Same rules would apply for where to put it.
| bool context::is_loaded(jtl::immutable_string const &module) | ||
| { | ||
| auto const atom{ | ||
| runtime::try_object<runtime::obj::atom>(__rt_ctx->loaded_libs_var->deref())->deref() |
There was a problem hiding this comment.
We're in the RT context. We shouldn't be using __rt_ctx.
| @@ -212,8 +214,7 @@ namespace jank::runtime | |||
| * moves the `llvm::Module` held in the `PartialTranslationUnit`. */ | |||
| if(truthy(compile_files_var->deref())) | |||
| { | |||
| auto module_name{ runtime::to_string(current_module_var->deref()) }; | |||
There was a problem hiding this comment.
So, aside from moving the set_is_loaded and is_loaded to RT context, which isn't a behavior change, this is actually the logical change.
We have a var to track the current module. Why is it not sufficient? You're using a parameter instead. This is suspicious to me, since we'll have other places in the code relying on the current_module_var holding the current module. Why are we not setting it in the loader when load .cpp modules. In fact, where are we setting it?
These are questions we need to understand, answer, and (if we're keeping this code) ultimately document. My gut tells me there's no reason we can't use the var for this.
|
Oops, it was supposed to be a draft :) |
Required to compile cpp backed module into object file