Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Stdlib linked list #491

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

Open
ChetanKarwa wants to merge 3 commits into fortran-lang:master
base: master
Choose a base branch
Loading
from ChetanKarwa:stdlib-linked-list

Conversation

@ChetanKarwa
Copy link
Member

@ChetanKarwa ChetanKarwa commented Aug 20, 2021

This PR is to submit the first draft of the linked list module for stdlib.
This is based upon #68 by Milan, #463 by me.

I have been working on this module as a part of my GSoC Project.
All my work was regularly updated on my personal repository that I have been sharing with everyone in my weekly report.
Link to my repository (here).
A bit detailed project report is available (here).
List of all the weekly Blogs - (here)

The module was tested in two compilers, intel oneAPI and gfortran.

milancurcic, zoziha, and francisco-rente reacted with rocket emoji
@awvwgk awvwgk added topic: container (Abstract) data structures and containers reviewers needed This patch requires extra eyes labels Aug 21, 2021
Copy link
Member

awvwgk commented Sep 13, 2021

How should we review this patch best? While there is an implementation I don't find a specification, examples or tests in this PR.

Copy link
Member

arjenmarkus commented Sep 15, 2021 via email

Hi Sebastian, Chetan and I can work on that - the documentation resides in his own repository together with the workplan, but let us distill a version that is suitable for reviewing the patch. We have a meeting planned for this friday, mainly for preparing the conference, but this may be a second topic. Op ma 13 sep. 2021 om 23:29 schreef Sebastian Ehlert < ***@***.***>:
...
How should we review this patch best? While there is an implementation I don't find a specification, examples or tests in this PR. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#491 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6YR3ZNUGNWGZWTMDXGTTUBZUKTANCNFSM5CRFTFJA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

@jvdp1 jvdp1 added the waiting for OP This patch requires action from the OP label Nov 10, 2021
Copy link
Member

awvwgk commented Dec 19, 2021

Any update on this feature?

Copy link
Member

arjenmarkus commented Dec 20, 2021 via email

I have not been in a position yet to follow on this. I want to contact @chetan karwa ***@***.***> about it, but my workload has not diminished enough that I can actually spare the energy yet. Op zo 19 dec. 2021 om 13:46 schreef Sebastian Ehlert < ***@***.***>:
...
Any update on this feature? — Reply to this email directly, view it on GitHub <#491 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6YR4ZYWPHQOBJUUCMVNDURXHZBANCNFSM5CRFTFJA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

Copy link
Contributor

@zoziha zoziha left a comment

Choose a reason for hiding this comment

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

This linked list is very good 👍 and can store almost any data type, I leave a little suggestion based on my limited knowledge, maybe not quite right :)

deallocate(current_node)
this_child_list%num_nodes = this_child_list%num_nodes - 1
end do

Copy link
Contributor

@zoziha zoziha Mar 2, 2022
edited by 14NGiestas
Loading

Choose a reason for hiding this comment

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

Suggested change
nullify (this_child_list%head, this_child_list%tail)

It seems that the head and tail of the linked list need to be blanked here, otherwise an error will be reported when a new push node is pushed again.

current_node = this_node
next_node => current_node%next
do
deallocate(current_node)
Copy link
Contributor

@zoziha zoziha Mar 2, 2022
edited by 14NGiestas
Loading

Choose a reason for hiding this comment

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

Suggested change
deallocate(current_node)
callcurrent_node%clear()
deallocate(current_node)

It seems that the destruction of the current node content should be added here.


if (index == node_index) then
! Return the pointer to item stored at specified index
return_item => current_node%item
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a routine can be abstracted here, allowing the user to customize the way to get the contents of the linked list?

...
generic :: get => get_node_at_index, &
 get_node_at_index_user
procedure, private :: get_node_at_index, &
 get_node_at_index_user
...
abstract interface
 subroutine get_value(this_item, return_item)
 class(*), intent(in) :: this_item
 class(*), intent(out) :: return_item
 end subroutine get_value
end interface
...
subroutine get_node_at_index_user(this_list, node_index, func, return_item)
 class(child_list), intent(inout) :: this_list
 integer, intent(in) :: node_index
 procedure(get_value) :: func
 class(*), intent(out) :: return_item
 type(node), pointer :: curr_node
 integer index
 curr_node => this_list%head
 index = 1
 do while (associated(curr_node))
 if (index == node_index) then
 call func(curr_node%item, return_item)
 nullify (curr_node)
 return
 end if
 curr_node => curr_node%next
 index = index + 1
 end do
 nullify (curr_node)
end subroutine get_node_at_index_user
...

The user can use it like this:

program main
 use linked_list_m, only: child_list, rk, ik
 implicit none
 real(rk) :: x
 integer(ik) :: n
 type(this_child) :: list
 print *, list%size()
 call list%push(1.0_rk)
 call list%push(2_ik)
 call list%get(1, get_value, x)
 call list%get(2, get_value, n)
 print *, x, n, list%size()
contains
 subroutine get_value(this_item, return_item)
 class(*), intent(in) :: this_item
 class(*), intent(out) :: return_item
 select type (this_item)
 type is (real(rk))
 select type (return_item)
 type is (real(rk))
 return_item = this_item
 class default
 print *, '*<ERROR>* get_value: type mismatch'
 end select
 type is (integer(ik))
 select type (return_item)
 type is (integer(ik))
 return_item = this_item
 class default
 print *, '*<ERROR>* get_value: type mismatch'
 end select
 class default
 print *, "*<ERROR>* get_value: invalid type"
 end select
 end subroutine get_value
end program main


end do
nullify(current_node)
nullify(return_item)

This comment was marked as abuse.

Comment on lines +54 to +57
class(*), intent(in), optional :: new_item

! allocating new_item to the new node's item
allocate(new_node%item, source=new_item)
Copy link
Contributor

Choose a reason for hiding this comment

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

source uses the optional argument new_item here, there may be memory access errors.

@14NGiestas 14NGiestas linked an issue Jul 5, 2022 that may be closed by this pull request
Copy link
Member

awvwgk commented Jul 30, 2022

I guess this PR has become stale. Since we already have a decent implementation here, should we try to update the PR, address the review comments and get it merged?

Copy link
Member

jvdp1 commented Jul 31, 2022

I guess this PR has become stale. Since we already have a decent implementation here, should we try to update the PR, address the review comments and get it merged?

Maybe @arjenmarkus / @chetan has some news/updates. Specs and tests are mainly missing.

awvwgk reacted with thumbs up emoji

Copy link
Member

I have (finally, sigh ;)) started setting up a test program. The documentation will follow with it.

sakamoti reacted with hooray emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zoziha zoziha zoziha left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

reviewers needed This patch requires extra eyes topic: container (Abstract) data structures and containers waiting for OP This patch requires action from the OP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Linked list

AltStyle によって変換されたページ (->オリジナル) /