Fix scoping issue #2161
Fix scoping issue #2161
Conversation
|
Ready. |
|
Thanks @Shaikh-Ubaid, very useful. @Thirumalai-Shaktivel, @czgdp1807 can you please review this and check if you see any issue with this approach? This might be a simpler and more robust approach than what we were using so far. |
|
Just removed the function |
Shaikh-Ubaid
force-pushed
the
fix_scoping2
branch
from
640881f
to
e715e83
Compare
| "The script is invoked as the main module and it has code to execute,\n" | ||
| "but `--disable-main` was passed so no code was generated for `main`.\n" | ||
| "We are removing all global executable code from ASR.", | ||
| diag::Level::Warning, diag::Stage::Semantic, {}) |
There was a problem hiding this comment.
I think we need to keep this warning?
There was a problem hiding this comment.
In the previous approach, we were removing the global executable code. I think therefore we were printing the warning that global code is being removed. In the current approach, we do not remove any global code (The global code is present inside the functions global_init() and global_stmts().). When disable main is enabled, we just do not execute the global code (that is, it is present but not executing). Since, there is no loss or removal of code, I guess we need not print the warning.
src/libasr/pass/global_stmts.cpp
Outdated
| /* a_return_var */ (return_var ? return_var_ref : nullptr), | ||
| (return_var ? ASR::abiType::BindC : ASR::abiType::Source), |
There was a problem hiding this comment.
This is fine, I think. If a tertiary operator doesn't suit our design, we can use some variable to store the values and pass them here.
| @@ -5,16 +5,16 @@ source_filename = "LFortran" | |||
| @1 = private unnamed_addr constant [2 x i8] c"\0A\00", align 1 | |||
| @2 = private unnamed_addr constant [5 x i8] c"%d%s\00", align 1 | |||
|
|
|||
| define void @__module__global_symbols__lpython_main_program() { | |||
| define void @__module___main_____main____global_statements() { | |||
There was a problem hiding this comment.
We need to handle this name mangling correctly.
There was a problem hiding this comment.
Got it. I guess we can work on it in a separate PR.
There was a problem hiding this comment.
Yup, this seems to be more robust, as we handle everything in the ASR itself.
This looks great! Thanks @Shaikh-Ubaid!
|
Thank you so much for the review @Thirumalai-Shaktivel! I appreciate it. |
|
@Shaikh-Ubaid can you please merge #2160 and then update this branch against master, so that we can see just the changes? I want to see if we need to test it with LFortran as well. |
Work towards Program_t only if disable_main is false
This makes global_init() and global_stmts() unique across modules
Remove unused function move_symbols_from_global_scope() The global_symbols pass that uses it was removed. This function is unused now.
Shaikh-Ubaid
force-pushed
the
fix_scoping2
branch
from
e715e83
to
a43632e
Compare
There was a problem hiding this comment.
I think it's fine. Thanks for fixing this @Shaikh-Ubaid. If there is a problem, we'll fix it later.
fixes #2146
In this PR the
main_moduleis also considered/treated as every othermoduleand added into the ASRTranslation_Unit_tas aModule_t.