Skip to content

Implement string comparison #744

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

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

AbdelrahmanKhaledd
Copy link
Collaborator

@AbdelrahmanKhaledd AbdelrahmanKhaledd commented Jul 7, 2022

Fixes #197

with basic testing a == "sd" it give me Stored value type does not match pointer operand type!, how to fix return type to be bool? or Is the problem something else?

code generation error: asr_to_llvm: module failed verification. Error:
Stored value type does not match pointer operand type!
  store i8* %4, i1* %s, align 8
 i1

@namannimmo10

@namannimmo10
Copy link
Collaborator

You are using lfortran_strop to call into C, but it assumes that the third argument is of type char**.
See lfortran_strrepeat for example, it assumes the second argument is of type int32_t. So I think you need a different function (maybe lfortran_strcmp) to handle string comparison.

@AbdelrahmanKhaledd
Copy link
Collaborator Author

You are using lfortran_strop to call into C, but it assumes that the third argument is of type char**. See lfortran_strrepeat for example, it assumes the second argument is of type int32_t. So I think you need a different function (maybe lfortran_strcmp) to handle string comparison.

i made lfortran_str_cmp to accept int8 as a result and it didn't work, i tried to make functions return value like str_len (last commit) and it didn't work either and, they gave me same message

code generation error: asr_to_llvm: module failed verification. Error:
Stored value type does not match pointer operand type!
  store i8 %3, i1* %s, align 1
 i1

is i1 refere to void?

@namannimmo10
Copy link
Collaborator

i1 is a boolean value (True or False), a 1-bit integer.

@namannimmo10
Copy link
Collaborator

This looks better now. Please add the tests covering all corner cases and test them with cpython and llvm.

@namannimmo10 namannimmo10 changed the title StringsCompare Implement string comparison Jul 9, 2022
@namannimmo10
Copy link
Collaborator

You also need to add this file in https://github.com/lcompilers/lpython/blob/main/integration_tests/CMakeLists.txt with labels, cpython and llvm.

@namannimmo10
Copy link
Collaborator

Please do a git rebase with the main branch rather than merge it with main. It leads to unnecessary commits, making a mess in the history.

$ git checkout main
$ git pull upstream main
$ git checkout stringsCompare
$ git rebase main   # if ever this leads to conflicts - resolve manually

@AbdelrahmanKhaledd
Copy link
Collaborator Author

AbdelrahmanKhaledd commented Jul 10, 2022

Please do a git rebase with the main branch rather than merge it with main. It leads to unnecessary commits, making a mess in the history.

$ git checkout main
$ git pull upstream main
$ git checkout stringsCompare
$ git rebase main   # if ever this leads to conflicts - resolve manually

i rebased the branch as you said but, it didn't squash the branch into one commit.

@namannimmo10
Copy link
Collaborator

You don't have to squash all the commits into one. Do git rebase -i main and it will show you the commits in an interactive mode. Now you can change pick to s to squash the commit, d to drop, and so on... For instance, you have three commits with the message, "Solve conflicts" that are not needed here, so change pick to s, so that we only have a relevant set of commits. When you're done, close the editor, and push it with --force.

@namannimmo10
Copy link
Collaborator

You also need to delete those 6 generated files.

assert s1 <= s2
assert s1 >= s2
s1 = "abcde"
assert s1 >= s2
Copy link
Collaborator

Choose a reason for hiding this comment

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

A test for the != operator would be nice too!

@@ -614,6 +614,53 @@ LFORTRAN_API void _lfortran_strcat(char** s1, char** s2, char** dest)
*dest = &(dest_char[0]);
}

#define MIN(x, y) (((x) < (y)) ? (x) : (y))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#define MIN(x, y) (((x) < (y)) ? (x) : (y))
#define MIN(x, y) ((x < y) ? x : y)

@namannimmo10
Copy link
Collaborator

Please make these changes and it should be ready afterward.

@@ -0,0 +1,696 @@
-- The C compiler identification is GNU 11.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file should not be committed.

@certik
Copy link
Contributor

certik commented Jul 11, 2022

After everything is ready, let's create a simple commit or commits and rebased on top of the latest master.

@AbdelrahmanKhaledd
Copy link
Collaborator Author

After everything is ready, let's create a simple commit or commits and rebased on top of the latest master.

it's ready now

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

That looks good to me, I think we can merge it.

Would you mind creating a nice set of patches for this? Or do you want me to just squash it using github?

@namannimmo10
Copy link
Collaborator

I squashed all the commits into one for now. @Abdelrahman-Kh-Fouad -- thanks a lot for working on this! If you still want to know how to squash commits, we can show you in our upcoming meeting.

@namannimmo10 namannimmo10 enabled auto-merge July 14, 2022 14:55
@namannimmo10 namannimmo10 merged commit 1c7430f into lcompilers:main Jul 14, 2022
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.

String comparison
3 participants