Skip to content

Generating sorting subroutines specific to character type with fypp #475

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 2 commits into from
Sep 16, 2021

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Jul 23, 2021

Previously all sorting subroutines for integer, real, and string were generated with fypp. character had its own subroutines.

I noticed that some checks were not present in character-based subroutines, while present for other types. Therefore, I modified the fypp instructions, such that the sorting subrotuines are generated from a same template for integer, real, string, and character.

With this strategy, if someone implements a derived type associated with >, <, ==,...., the sorting subroutines can be easily implemented for this DT by modifying only the fypp instructions.

Additional reviewer: @wclodius2

@jvdp1 jvdp1 requested review from milancurcic, ivan-pi and awvwgk July 23, 2021 18:57
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

I'm always happy to see negative line count patches. Looks good to me.

@awvwgk awvwgk added the reviewers needed This patch requires extra eyes label Jul 31, 2021
Copy link
Contributor

@gareth-nx gareth-nx left a comment

Choose a reason for hiding this comment

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

This is really nice -- in addition to the code simplification I think there are lessons on using fypp that would be nice to share more broadly.

@jvdp1
Copy link
Member Author

jvdp1 commented Aug 22, 2021

@gareth-nx Thank you for your review. I believed I answered all your comments.

@gareth-nx
Copy link
Contributor

@jvdp1 Thanks for the changes.

If anyone with commit rights has time to assist, note this patch seems very straightforward (it is only code simplification) and has been reviewed twice -- but we need another reviewer with commit rights to move it forward. @milancurcic @ivan-pi

@jvdp1
Copy link
Member Author

jvdp1 commented Sep 16, 2021

@milancurcic @ivan-pi @LKedward Could one of you review this PR, and merge it if you agree with it, please?

@awvwgk
Copy link
Member

awvwgk commented Sep 16, 2021

With two approvals this patch is ready to go. I'll go ahead and merge it now.

@awvwgk awvwgk merged commit 21e20f0 into fortran-lang:master Sep 16, 2021
@jvdp1 jvdp1 deleted the sort_simp branch September 16, 2021 19:02
@awvwgk awvwgk removed the reviewers needed This patch requires extra eyes label Sep 25, 2021
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