Add builtin functions #2651

farah-salama · 2024-04-14T20:51:43Z

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

ubaidsk

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.

ubaidsk · 2024-04-15T11:16:55Z

tests/reference/run_dbg-test_raise_01-dfd86ca.stderr

-  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`


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.

I did as you suggested and this error appeared, I guess I missed something while implementing the function but I can't figure out what exactly

@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.

ubaidsk · 2024-04-16T08:56:44Z

src/lpython/semantics/python_ast_to_asr.cpp

+                value.loc = loc;
+                value.m_value = args[0].m_value;
+                fn_args.push_back(al, value);
+            }


This handles runtime implementation. We also need to support compile-time implementation inside

lpython/src/lpython/semantics/python_ast_to_asr.cpp

Line 6909 in 7d0cfdb

void handle_constant_string_attributes(std::string &s_var,

.

kmr-srbh

I shared some comments below. The PR looks good.

It would be nice to have #2652 resolved as it can speed up things.

kmr-srbh · 2024-04-20T03:22:28Z

src/runtime/lpython_builtin.py

+                for i in range(tabsize-(col % tabsize)):
+                    result += ' '
+            col = 0
+        elif (c == '\n') | (c == '\r'):


Suggested change

elif (c == '\n') | (c == '\r'):

elif c == '\n' or c == '\r':

kmr-srbh · 2024-04-20T03:35:02Z

src/runtime/lpython_builtin.py

+    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


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.

src/runtime/lpython_builtin.py

kmr-srbh · 2024-04-20T03:48:35Z

integration_tests/test_str_attributes.py

@@ -472,6 +472,24 @@ def is_numeric():
    assert "".isnumeric() == False
    assert "ab2%3".isnumeric() == False

+def center():


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.

@farah-salama please update the test references also.

Could you please rebuild and update the test references again?

kmr-srbh · 2024-04-21T13:58:15Z

@farah-salama the print statements existed for checking the tests. Please revert that change.

This reverts commit 1a9a70c.

farah-salama · 2024-04-21T14:55:01Z

@Shaikh-Ubaid @kmr-srbh Thank you so much for your help.
I tried to handle the compile-time implementation for the functions in handle_constant_string_attributes but I couldn't find a similar function implemented that takes an integer argument and I faced many errors that I couldn't solve so I changed the test cases to test the run time implementation only, I would appreciate it though if someone could tell me how to handle the compile-time implementation for such functions.

kmr-srbh · 2024-04-21T15:42:38Z

@farah-salama I present the method to implement center. You may similarly follow to implement expandtabs.

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 fillchar is an optional parameter, we need an if block to check for args.size() again. When we have 1 argument, we handle only width. When we have 2 arguments, we handle both.

You can check for width being an integer type like:

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 args[1].m_value when we have 2 arguments.

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 nullptr:

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 width parameter of center, we can do:

ASR::IntegerConstant_t* width_arg = ASR::down_cast<ASR::IntegerConstant_t>(arg1);
int64_t width = width_arg->m_n;

fillchar is an optional parameter so we declare it as std::string fillchar = " "; when only 1 argument is provided. We follow the above step to store it as a StringConstant_t* when we have 2 arguments.

Now you need to implement the same logic you have used for implementing the method.
We need an else clause to handle cases when we have either one or all arguments non-constants. Inside that clause, we make a call to the _lpython_str_center function which you have already implemented. You may have a look at how endswith does this.

Finally, make a new StringConstant_t with the obtained value and set it to tmp.

This is a rough sketch, please modify it accordingly.

ubaidsk · 2024-04-25T20:04:43Z

@farah-salama Please mark "Ready for review" when ready.

ubaidsk

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 !

src/lpython/semantics/python_ast_to_asr.cpp

ubaidsk · 2024-04-26T02:53:22Z

src/runtime/lpython_builtin.py

+    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")


Considering #2666, I think this will not print the error message. But I think it is fine for now. The error message should start printing as soon as we fix #2666.

ubaidsk · 2024-04-26T02:54:45Z

src/runtime/lpython_builtin.py

+                for i in range(iterations):
+                    result += ' '


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]>

farah-salama added 6 commits April 14, 2024 22:15

add center and expandtabs tests

c4be5c9

add _lpython_str_center and _lpython_str_expandtabs

b8225df

add _lpython_str_center and _lpython_str_expandtabs

8e45dd0

fix str.center()

6c481ce

fix typo in line 6924

1550703

update test references

cad68e0

ubaidsk reviewed Apr 15, 2024

View reviewed changes

farah-salama and others added 2 commits April 15, 2024 14:26

Merge branch 'lcompilers:main' into add-builtin-functions

d87f0fa

update tests references

3aa3dbe

ubaidsk reviewed Apr 16, 2024

View reviewed changes

kmr-srbh suggested changes Apr 20, 2024

View reviewed changes

farah-salama and others added 6 commits April 21, 2024 15:18

Merge branch 'main' into add-builtin-functions

c23d29d

edit if condition in str.expandtabs

8e9fdf5

edit center and expandtabs tests

f947a3d

update test references

7964ab4

remove unnecessary print statements

1a9a70c

fix str.expandtabs

0c2bcab

Revert "remove unnecessary print statements"

42628ca

This reverts commit 1a9a70c.

farah-salama added 2 commits April 21, 2024 18:25

edit str.expandtabs

359bf27

edit str.expandtabs

b9de14e

farah-salama and others added 2 commits April 25, 2024 23:28

edit str.center

decfe10

Merge branch 'main' into add-builtin-functions

b2656c3

farah-salama marked this pull request as ready for review April 25, 2024 21:52

ubaidsk reviewed Apr 26, 2024

View reviewed changes

ubaidsk added 2 commits April 26, 2024 08:28

Formatting changes

eef9923

Formatting changes

3772abc

Refactor to use len() once

a95406a

Co-authored-by: Saurabh Kumar <[email protected]>

ubaidsk approved these changes Apr 26, 2024

View reviewed changes

ubaidsk enabled auto-merge (squash) April 26, 2024 03:04

ubaidsk merged commit 5f7ef49 into lcompilers:main Apr 26, 2024

Add builtin functions #2651

Add builtin functions #2651

Conversation

farah-salama commented Apr 14, 2024

Functions

Testing

Uh oh!

ubaidsk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmr-srbh Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmr-srbh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmr-srbh Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kmr-srbh Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmr-srbh commented Apr 21, 2024

Uh oh!

farah-salama commented Apr 21, 2024

Uh oh!

kmr-srbh commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ubaidsk commented Apr 25, 2024

Uh oh!

ubaidsk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kmr-srbh Apr 15, 2024 •

edited

Loading

kmr-srbh left a comment •

edited

Loading

kmr-srbh Apr 20, 2024 •

edited

Loading

kmr-srbh Apr 20, 2024 •

edited

Loading

kmr-srbh commented Apr 21, 2024 •

edited

Loading