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.

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

Issue with stdlib_sorting #428

Merged
merged 3 commits into from
Jun 13, 2021
Merged

Issue with stdlib_sorting #428

merged 3 commits into from
Jun 13, 2021

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Jun 5, 2021

The option -fcheck=all of gfortran triggered a problem with the suboutine insertion_sort, when i becomes equal to 0, and tried to access array( i-1 ) (while the lower bound of array is 0.

The solution copied what was already implemented in stdlib_sorting_sort.fypp.

@jvdp1 jvdp1 requested a review from awvwgk June 5, 2021 17:49
@jvdp1
Copy link
Member Author

jvdp1 commented Jun 5, 2021 •
edited
Loading

To review also: @wclodius2

jvdp1 added a commit to jvdp1/stdlib that referenced this pull request Jun 5, 2021
@jvdp1
median: mv to sort due to issue fortran-lang#428
935949e
@jvdp1 jvdp1 mentioned this pull request Jun 5, 2021
Merged
4 tasks
gareth-nx
gareth-nx reviewed Jun 6, 2021
View reviewed changes
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.

Looks good to me, except (possibly) for the change in repeat, which I don't understand.

wclodius2
wclodius2 approved these changes Jun 6, 2021
View reviewed changes
Copy link
Contributor

@wclodius2 wclodius2 left a comment

Choose a reason for hiding this comment

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

The changes to stdlib_sorting_ord_sort.fyppand stdlib_sorting_sort_index.fypp are all appropriate. The change to repeat in test_sorting.f90 is useful if the runtime is too long. The rest of the changes to test the changes the other codes are appropriate.

@jvdp1 jvdp1 requested review from ivan-pi and milancurcic June 6, 2021 09:58
awvwgk
awvwgk approved these changes Jun 6, 2021
View reviewed changes
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.

Looks good to me

@awvwgk awvwgk added the reviewers needed This patch requires extra eyes label Jun 9, 2021
@jvdp1
Copy link
Member Author

jvdp1 commented Jun 12, 2021

Could someone with write access review this PR that fixes a bug in stdlib_sort?

LKedward
LKedward approved these changes Jun 12, 2021
View reviewed changes
Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Cheers, LGTM 👍

@milancurcic
Copy link
Member

Reviews are positive, so I'll merge. Thanks!

@milancurcic milancurcic merged commit f92ba28 into fortran-lang:master Jun 13, 2021
@awvwgk awvwgk removed the reviewers needed This patch requires extra eyes label Jun 15, 2021
@jvdp1 jvdp1 deleted the issue_sorting branch December 22, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gareth-nx gareth-nx gareth-nx left review comments

@LKedward LKedward LKedward approved these changes

@awvwgk awvwgk awvwgk approved these changes

@wclodius2 wclodius2 wclodius2 approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@jvdp1 @milancurcic @LKedward @awvwgk @wclodius2 @gareth-nx