Skip to content

Add str() to casting #797

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jul 28, 2022
Merged

Add str() to casting #797

merged 8 commits into from
Jul 28, 2022

Conversation

AbdelrahmanKhaledd
Copy link
Collaborator

@AbdelrahmanKhaledd AbdelrahmanKhaledd commented Jul 19, 2022 •
edited
Loading

About #1

@certik
Copy link
Contributor

certik commented Jul 19, 2022

I think this should be implemented using an approach similar to #754

@namannimmo10
Copy link
Collaborator

@Abdelrahman-Kh-Fouad -- any updates?

@AbdelrahmanKhaledd
Copy link
Collaborator Author

@Abdelrahman-Kh-Fouad -- any updates?

i added float conversion to llvm.

certik
certik reviewed Jul 24, 2022
View reviewed changes
certik
certik reviewed Jul 25, 2022
View reviewed changes
@certik
Copy link
Contributor

certik commented Jul 25, 2022

Make sure you resolve the conflict in src/runtime/impure/lfortran_intrinsics.c.

@AbdelrahmanKhaledd AbdelrahmanKhaledd force-pushed the str()_for_other_inputs branch 3 times, most recently from 2817d40 to d4e7532 Compare July 26, 2022 19:19
@certik certik requested review from czgdp1807 and namannimmo10 July 26, 2022 20:33
@certik
Copy link
Contributor

certik commented Jul 26, 2022

It seems ok, I'll let @czgdp1807 and @namannimmo10 to review it in details. Note that there are still some conflicts:

src/lpython/semantics/python_comptime_eval.h
src/runtime/impure/lfortran_intrinsics.c

namannimmo10
namannimmo10 previously requested changes Jul 26, 2022
View reviewed changes
@AbdelrahmanKhaledd AbdelrahmanKhaledd force-pushed the str()_for_other_inputs branch 2 times, most recently from f5cbd48 to e875222 Compare July 27, 2022 04:48
@namannimmo10
Copy link
Collaborator

Could you resolve the merge conflicts? Let us know if you need any help with that.

czgdp1807
czgdp1807 previously requested changes Jul 27, 2022
View reviewed changes
namannimmo10
namannimmo10 reviewed Jul 27, 2022
View reviewed changes
@certik
Copy link
Contributor

certik commented Jul 27, 2022

It looks pretty good. What is the status of this --- is this ready for review?

@AbdelrahmanKhaledd
Copy link
Collaborator Author

It looks pretty good. What is the status of this --- is this ready for review?

Yes

@AbdelrahmanKhaledd AbdelrahmanKhaledd changed the title Add str() for f32 (naive algo) Add str() to casting Jul 28, 2022
namannimmo10
namannimmo10 reviewed Jul 28, 2022
View reviewed changes
namannimmo10
namannimmo10 reviewed Jul 28, 2022
View reviewed changes
@namannimmo10 namannimmo10 dismissed stale reviews from czgdp1807 and themself July 28, 2022 18:48

resolved

@namannimmo10
Copy link
Collaborator

This looks good enough to merge for now. I can improve this in a subsequent PR.
Thanks for your hard work on this PR, @Abdelrahman-Kh-Fouad.

@namannimmo10 namannimmo10 merged commit 327eff4 into lcompilers:main Jul 28, 2022
@AbdelrahmanKhaledd AbdelrahmanKhaledd deleted the str()_for_other_inputs branch July 28, 2022 18:56
@namannimmo10
Copy link
Collaborator

Yikes. Looks like we get some warnings after merging this PR. I fix those at #838.

@AbdelrahmanKhaledd
Copy link
Collaborator Author

Yikes. Looks like we get some warnings after merging this PR. I fix those at #838.

sorry about that :)

@namannimmo10 namannimmo10 mentioned this pull request Jul 29, 2022
Merged
@AbdelrahmanKhaledd AbdelrahmanKhaledd mentioned this pull request Aug 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@certik certik certik left review comments

@namannimmo10 namannimmo10 namannimmo10 left review comments

@czgdp1807 czgdp1807 Awaiting requested review from czgdp1807

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AbdelrahmanKhaledd @certik @namannimmo10 @czgdp1807