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

Support to print lists in REPL #2752

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

Vipul-Cariappa
Copy link
Contributor

No description provided.

@Vipul-Cariappa
Copy link
Contributor Author

@Shaikh-Ubaid, Could you please have a look at this and let me know if it requires any changes?
I have also refactored global_stmts.cpp.
I have made one more change; I am assigning the last top-level expression of a code block to a global variable. I am then getting a pointer to this global variable to print it to the console. Another reason for assigning it to a global variable is to make sure it is not deallocated, for example

>>> [1, 2, 3]

The list in the above example would be allocated in the global_stmts function, and it would be deallocated when global_stmts returns. But we want it to be deallocated after we print the list.

It is also the pre-requisite for the _ variable, to get the last code block's value. Regarding deallocation; we would want to delay it further.

@@ -319,4 +360,105 @@ Result<std::unique_ptr<LLVMModule>> PythonCompiler::get_llvm3(
#endif
}

void print_type(ASR::ttype_t *t, void *data, std::string &result);

std::string PythonCompiler::string_aggregate_type(const struct EvalResult &r) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this function converting a given aggregate type to string? If yes, it should be called aggregate_type_to_string() or cnvrt_aggregate_type_to_string() or something similar. string_aggregate_type doesn't give any good idea of what the function does and seems rather misleading.

// idx++;
// return_var_ref = nullptr;
// return_var = nullptr;
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented code should be removed.


llvm::Type *type = nullptr;
ASR::symbol_t *f = symbol_table->get_symbol("_" + run_fn);
if ((return_type == "struct") && (f)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slightly confused here. What are we processing inside this if and why do we need this processing?

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

Shared few comments. Rest seems good.

@Vipul-Cariappa Vipul-Cariappa marked this pull request as ready for review July 6, 2024 03:33
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

Looks good to me. Great work @Vipul-Cariappa! Left a minor comment. It can be fixed in a separate PR.

@@ -319,4 +344,134 @@ Result<std::unique_ptr<LLVMModule>> PythonCompiler::get_llvm3(
#endif
}

void print_type(ASR::ttype_t *t, void *data, std::string &result);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this declaration? If we need it then a better place for it could be python_evaluator.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The declaration is needed. print_type is defined at the end after all the definitions of PythonCompiler class. print_type is not supposed to be called by any other part. It is a helper function for aggregate_type_to_string.

@Shaikh-Ubaid
Copy link
Collaborator

@Vipul-Cariappa please resolve the conflicts and we can merge this.

@Shaikh-Ubaid Shaikh-Ubaid merged commit ba07d7b into lcompilers:main Jul 9, 2024
15 checks passed
@Vipul-Cariappa Vipul-Cariappa deleted the top-lists branch July 27, 2024 12:23
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