Skip to content

Conversation

AbdelrahmanKhaledd
Copy link
Collaborator

@AbdelrahmanKhaledd AbdelrahmanKhaledd commented Aug 2, 2022

Related to #281

@AbdelrahmanKhaledd AbdelrahmanKhaledd force-pushed the int_bit_length branch 2 times, most recently from 19420c8 to 7de8d29 Compare August 3, 2022 21:38
@AbdelrahmanKhaledd
Copy link
Collaborator Author

@namannimmo10 i've implemented it on llvm.

@namannimmo10
Copy link
Collaborator

Please add test before we review the code.

@namannimmo10
Copy link
Collaborator

Thanks for the PR!

We should:
a) resolve conflicts.
b) fix the warnings we get while compiling LPython
c) maybe rename IntBitLength to IntegerBitLen.
d) remove redundant test_builtin_str.py from the diff.

@AbdelrahmanKhaledd
Copy link
Collaborator Author

AbdelrahmanKhaledd commented Aug 4, 2022

Thanks for the PR!

We should: a) resolve conflicts. b) fix the warnings we get while compiling LPython c) maybe rename IntBitLength to IntegerBitLen. d) remove redundant test_builtin_str.py from the diff.

I rebased and changed commits messages.

Copy link
Collaborator

@namannimmo10 namannimmo10 left a comment

Choose a reason for hiding this comment

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

I left some comments. Looks good though.

@AbdelrahmanKhaledd
Copy link
Collaborator Author

AbdelrahmanKhaledd commented Aug 8, 2022

@namannimmo10
I added it to constant integer and to other kinds of integers.

@AbdelrahmanKhaledd AbdelrahmanKhaledd force-pushed the int_bit_length branch 3 times, most recently from ebe92dd to 091464a Compare August 8, 2022 22:45
@namannimmo10
Copy link
Collaborator

Please fix the warnings if you are getting any. We'd have to do that later otherwise.

@namannimmo10
Copy link
Collaborator

@certik @czgdp1807 -- PTAL.

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.

I think that looks good. After comments are addressed, it can be merged. Git history should be polished.

@AbdelrahmanKhaledd
Copy link
Collaborator Author

@namannimmo10
i have merged main previously, now i rebasing the branch but as you see all commits from main now here and i solved conflicts, is that the correct?

@namannimmo10
Copy link
Collaborator

If you do git rebase main (given that your local main branch is up to date), you'd see merge conflicts. You need to resolve them manually.

@namannimmo10
Copy link
Collaborator

@Abdelrahman-Kh-Fouad -- now we can add the string-related methods.

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.

3 participants