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

Fix logical comparsion for string #2699

Merged
merged 3 commits into from
May 19, 2024
Merged

Conversation

hankluo6
Copy link
Contributor

Fixes #2615

The problem is that _lpython_str_compare_eq requires pointer to pointer to char as parameters. However, the previous implementation passed a null pointer, leading to a segmentation fault.

@kmr-srbh
Copy link
Contributor

Thanks for this @hankluo6! The PR looks good! 👍

Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

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

I think these changes look good considering the llvm backend. So we could have this merged.
But we could do that once we make note of the fact that test_logical_assignment & test_logical_compare don't work with the c backend.

def main0():
    s_var1: str = "Hello"
    s_var2: str = "LPython"

    print(s_var1 or s_var2)

(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython examples/expr2.py --backend=c
expr2__tmp__generated__.c: In function ‘main0’:
expr2__tmp__generated__.c:21:14: warning: format ‘%s’ expects argument of type ‘char *’, but argument 2 has type ‘int’ [-Wformat=]
   21 |     printf("%s\n", s_var1 || s_var2);
      |             ~^     ~~~~~~~~~~~~~~~~
      |              |            |
      |              char *       int
      |             %d
Segmentation fault

Could you check if we have an issue raised for the same ? If not could you please raise an issue for the same. Once that is done, let's merge this and we could then try addressing the issues through the C backend too !

@hankluo6
Copy link
Contributor Author

Thank you @anutosh491! I raised an issue #2708 for this.

@anutosh491
Copy link
Collaborator

Could you maybe add a TODO here saying that we can add the C backend once the above issue you raised has been fixed ?

RUN(NAME test_logical_compare LABELS cpython llvm llvm_jit)
RUN(NAME test_logical_assignment LABELS cpython llvm llvm_jit)

I guess that would be sufficient enough to merge this !

@anutosh491 anutosh491 enabled auto-merge (squash) May 19, 2024 04:35
@anutosh491 anutosh491 disabled auto-merge May 19, 2024 04:42
@anutosh491 anutosh491 merged commit 04de1f1 into lcompilers:main May 19, 2024
14 checks passed
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.

Bug: Logical operation on str variables fail
3 participants