Conversation
ubaidsk
left a comment
There was a problem hiding this comment.
I would suggest rebasing your PR over the latest main, rebuilding the project and updating the reference tests.
At a high level this seems good. I will review again after the rebase.
| File "tests/runtime_errors/test_raise_01.py", line 2 | ||
| raise | ||
| ERROR STOP | ||
| semantic error: The `runtime stacktrace` is not enabled. To get the stacktraces, re-build LPython with `-DWITH_RUNTIME_STACKTRACE=yes` |
There was a problem hiding this comment.
You should build the lpython project with -DWITH_RUNTIME_STACKTRACE=yes. You can simply use the provided build0.sh and build1.sh files for building which has this options already setup.
There was a problem hiding this comment.
@farah-salama the issue occurs because you have not currently implemented the method for compile-time strings. You can have a look at how startswith is implemented inside the handle_constant_string_attributes method in python_ast_to_asr.cpp and add support for them.
| value.loc = loc; | ||
| value.m_value = args[0].m_value; | ||
| fn_args.push_back(al, value); | ||
| } |
There was a problem hiding this comment.
This handles runtime implementation. We also need to support compile-time implementation inside
.
src/runtime/lpython_builtin.py
Outdated
| for i in range(tabsize-(col % tabsize)): | ||
| result += ' ' | ||
| col = 0 | ||
| elif (c == '\n') | (c == '\r'): |
There was a problem hiding this comment.
| elif (c == '\n') | (c == '\r'): | |
| elif c == '\n' or c == '\r': |
| i: i32 | ||
| for i in range(left_padding): | ||
| result += fillchar | ||
| right_padding: i32 = width - left_padding | ||
| result += s | ||
| for i in range(right_padding): | ||
| result += fillchar |
There was a problem hiding this comment.
We can utilize the new improvement in string repeat added a few days ago and simplify this to just:
right_padding: i32 = width - left_padding
result += fillchar * left_padding
result += s
result += fillchar * right_padding#2652 needs to be addressed to achieve it. For now we may keep the current for loop.
| assert "".isnumeric() == False | ||
| assert "ab2%3".isnumeric() == False | ||
|
|
||
| def center(): |
There was a problem hiding this comment.
The current implementation supports runtime strings. Please add tests for them. By runtime strings, I mean a string stored in a variable. You can have a look at how is_upper() is tested.
There was a problem hiding this comment.
@farah-salama please update the test references also.
There was a problem hiding this comment.
Could you please rebuild and update the test references again?
|
@farah-salama the print statements existed for checking the tests. Please revert that change. |
This reverts commit 1a9a70c.
|
@Shaikh-Ubaid @kmr-srbh Thank you so much for your help. |
|
@farah-salama I present the method to implement We first check for the number of arguments to the function. if (args.size() != 1 || args.size() != 2) {
throw SemanticError("str.center() takes 1 or 2 arguments",
loc);
}We next move on to check the argument types. As You can check for ASR::expr_t *arg1 = args[0].m_value;
ASR::ttype_t *type = ASRUtils::expr_type(arg1);
if (!ASRUtils::is_integer(*type)) {
throw SemanticError("argument 'width' to str.center() must be of type int",
arg1->base.loc);
}We follow the above step again to check for We next check whether the provided arguments are compile-time constants. Handle this according to the arguments you are handling above. We do so by checking if they are not all if (ASRUtils::expr_value(arg1) != nullptr) {...}We next store their compile time values. We do this here because we need all the arguments to be a compile-time constant to move ahead. For the ASR::IntegerConstant_t* width_arg = ASR::down_cast<ASR::IntegerConstant_t>(arg1);
int64_t width = width_arg->m_n;
Now you need to implement the same logic you have used for implementing the method. Finally, make a new This is a rough sketch, please modify it accordingly. |
|
@farah-salama Please mark "Ready for review" when ready. |
ubaidsk
left a comment
There was a problem hiding this comment.
This looks good to me. I shared some comments and code suggestions. Let's do the code suggestions and merge this. Great work @farah-salama ! Thanks for the help @kmr-srbh !
| The original string is returned if width is less than or equal to len(s). | ||
| """ | ||
| if(len(fillchar) != 1): | ||
| raise TypeError("The fill character must be exactly one character long") |
| for i in range(iterations): | ||
| result += ' ' |
There was a problem hiding this comment.
For now this is fine, but we can later refactor this to simply use * operator.
Co-authored-by: Saurabh Kumar <[email protected]>
) * add center and expandtabs tests * add _lpython_str_center and _lpython_str_expandtabs * add _lpython_str_center and _lpython_str_expandtabs * fix str.center() * fix typo in line 6924 * update test references * update tests references * edit if condition in str.expandtabs * edit center and expandtabs tests * update test references * remove unnecessary print statements * fix str.expandtabs * Revert "remove unnecessary print statements" This reverts commit 1a9a70c. * edit str.expandtabs * edit str.expandtabs * edit str.center * Formatting changes * Formatting changes * Refactor to use len() once Co-authored-by: Saurabh Kumar <[email protected]> --------- Co-authored-by: Shaikh Ubaid <[email protected]> Co-authored-by: Saurabh Kumar <[email protected]>
) * add center and expandtabs tests * add _lpython_str_center and _lpython_str_expandtabs * add _lpython_str_center and _lpython_str_expandtabs * fix str.center() * fix typo in line 6924 * update test references * update tests references * edit if condition in str.expandtabs * edit center and expandtabs tests * update test references * remove unnecessary print statements * fix str.expandtabs * Revert "remove unnecessary print statements" This reverts commit 1a9a70c. * edit str.expandtabs * edit str.expandtabs * edit str.center * Formatting changes * Formatting changes * Refactor to use len() once Co-authored-by: Saurabh Kumar <[email protected]> --------- Co-authored-by: Shaikh Ubaid <[email protected]> Co-authored-by: Saurabh Kumar <[email protected]>

Functions
str.center(): Return centered in a string of length width. Padding is done using the specified fillchar (default is an ASCII space).str.expandtabs(): Return a copy of the string where all tab characters are replaced by one or more spaces, depending on the current column and the given tab size.Testing
Tests were added in
integration_tests/test_str_attributes.py