-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
feat: init checks for dependency from current scope implemented #2327
feat: init checks for dependency from current scope implemented #2327
Conversation
Thanks! I think you need to resolve some conflicts. |
@arteevraina Let's meet on Friday and get it done. |
Sure. @czgdp1807 |
I think you need to resolve conflicts for the tests to run. |
a1b42ab
to
8ca308d
Compare
50316b9
to
6c4113c
Compare
@czgdp1807, @arteevraina what's the status of this work? Let's figure out how to wrap this up and finish it. |
@certik all of the integration tests are passing now. There are few CI errors which I am figuring out currently and will discuss in my upcoming meeting with @czgdp1807 |
Awesome, excellent! Keep me updated and if you need help from me, let me know. |
I had the meeting with @czgdp1807 today and we discussed that there are still some tests that fail other than the integration ones. So, I will try to resolve the issue with those failing tests and run the combination of these commands : cc: @certik |
@arteevraina ok. Please ping me if you can't figure out how to finish this. |
6c4113c
to
6f61a4e
Compare
4dbf0c0
to
4c63659
Compare
…function's symbol table
You have to use LLVM 11. You are updating tests with a later version I think, so they look slightly different. |
} \ | ||
if (!is_parent && !ASR::is_a<ASR::ExternalSymbol_t>(*final_sym)) { \ | ||
current_function_dependencies.push_back(al, dep_name); \ | ||
} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to write these using functions? Macros are always worse, unless there is no other easy way.
If there is no way using functions, then you need to prefix the macros with LCOMPILERS_*
to reduce a chance of collision. In general having macros in header files is a bad idea.
Overall I think this looks good. @Thirumalai-Shaktivel, @czgdp1807 can you please also review this? @arteevraina if you could now update your LPython PR to be identical with regards to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just re-write the git history and merge. Thanks for this. A lot of effort went into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@arteevraina I appreciate the contributions. I am confused about which specific bug in LFortran this PR fixes. Like we previously discussed, have you had the opportunity to create a minimum possible example such that
|
I think this PR and the corresponding PR in LPython fix a bug in ASR that it had incorrect dependencies. The minimal reproducible example is a stricter ASR check, as implemented in this PR. Without the other changes in this PR, tons of tests will fail. So that's why the changes are required. |
src/libasr/asr_verify.cpp
Outdated
if (dep_sym_global != nullptr && ASRUtils::symbol_parent_symtab(dep_sym_global)->parent == nullptr) { | ||
// This is a global scope symbol, which we allow for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this if
condition will never be true or is a dead condition because dep_sym_global
will always be nullptr
.
We have ASR::symbol_t* dep_sym = sym->resolve_symbol(found_dep);
and we are inside if (dep_sym == nullptr) {
. Now, we have ASR::symbol_t* dep_sym_global = sym->resolve_symbol(found_dep);
. I think dep_sym
and dep_sym_global
point to the same thing. As dep_sym
is nullptr
(since we are inside the if), I think
dep_sym_globalwould also be
nullptr`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I feel the same thing. If you are planning to search the symbol in parent symbol table, you can try sym->parent->get_symbol(found_dep)
and then do sym->parent->resolve_symbol(found_dep)
or sym->resolve_symbol(found_dep)
Here, get_symbol
searches only in the current symbol table, but resolve_symbol
recursively searches in the current, parent, grandparent, ... symbol tables.
// Get the x parent symtab. | ||
SymbolTable *sym = x.m_symtab->parent; | ||
|
||
// Dependencies of the function should be from function's parent symbol table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious to know why dependencies should be from parent's symbol table. Can they be from the grand-parent's /children's symbol table?
What if we call a function from other module? Do we consider it as a dependency? (If we do not consider it as a dependency, please explain why not)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read the description at #2327 (comment). How do dependencies work for call to functions from other modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use ExternalSymbol. If it lives in the current symbol table, then you must depend on it. If it lives in the parent/grandparent, then you don't explicitly depend on it.
@arteevraina is the LPython PR and the LFortran PR synchronized? Is this ready for final review? |
@czgdp1807 can you please review this? |
Ref: lcompilers/lpython#2167 (comment)
LPython: lcompilers/lpython#2167