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.

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

Revival string list #470

Merged
merged 41 commits into from
Aug 26, 2021
Merged

Conversation

aman-godara
Copy link
Member

@aman-godara aman-godara commented Jul 21, 2021

This PR is based upon #311 by Arjen. Since 311 is very long, I decided to break it into parts and making it easier for others to review and for me to concentrate on one thing at a time.
How to Start Reviewing by gareth.

Previous discussions (in order of time):
Linked Issue: #268
string list: #269
string list new: #311

Time Line:

  • 1. Arjen's kickstart
  • 2. Corrected a bug in append operator(//)
  • 3. Refactored code to use stdlib's string_type, Provided support for allocatable character (character(len=:), allocatable)
  • 4. Used concept of forward backward indexes
  • 5. added copy function
  • 6. Blocked integer indexes to stringlist use only stringlist_index_type indexes
  • 7. to_current_idxn (get) vs to_future_at_idxn (insert_at)
  • 8. added == and /= operator
  • 9. Improve test cases for insert_at, append and prepend
  • 10. Add Documentation

Inviting: @zoziha, @St-Maxwell, others are welcomed as well.

@aman-godara

This comment has been minimized.

@milancurcic milancurcic mentioned this pull request Jul 22, 2021
@aman-godara
Copy link
Member Author

aman-godara commented Jul 23, 2021

Explanation of forward and backward indexes:
forward index: fidx
backward index: bidx

After inserting an element at fidx( x ), the added element will appear at index fidx( x ) i.e. at xth position when counted from left to right.
After inserting an element at bidx( y ), the added element will appear at index bidx( y ) i.e. yth position when counted from right to left.
inserting at fidx( 1 ) is equivalent to inserting at bidx( len + 1 )
inserting at bidx( 1 ) is equivalent to inserting at fidx( len + 1 )

Inserting an element at HEAD is like inserting at fidx( 1 ) and inserting an element at TAIL is like inserting at bidx( 1 )

backward index is like the forward index of the reversed form of the list.


index type ... --- A B .. Y Z --- ...
integer ... --- 1 2 .. len - 1 len --- ...
forward ... fidx( 0 ) fidx( 1 ) fidx( 2 ) .. fidx( len - 1 ) fidx( len ) fidx( len + 1 ) ...
backward ... bidx( len + 1 ) bidx( len ) bidx( len - 1 ) .. bidx( 2 ) bidx( 1 ) bidx( 0 ) ...

If you want to insert an element new_element before integer index q use the forward index of integer index q. If you want to insert an element new_element after integer index q use the backward index of integer index q.

Currently there is NO function in the PR which converts integer index to forward index or backward index [not to be confused with fidx and bidx of PR]

arjenmarkus and others added 4 commits July 24, 2021 11:59
The API for the module to manipulate lists of strings has been discussed and this has resulted in the current implementation
There was a typo in some comments - corrected
…ib's string_type, provided suppport for allocatable character
@zoziha

This comment has been minimized.

@aman-godara

This comment has been minimized.

@aman-godara
Copy link
Member Author

aman-godara commented Jul 24, 2021

Apart from reviewing code, you can also leave your opinion and feedback on the design. Coming up with a good design is the major challenge in this PR.

The design that I have proposed has problems (I agree) one of them is: no one would prefer to use these forward and backward indexes over integer indexes. Having these stringlist_index_type indexes just makes code more verbose by calling fidx and bidx function again and again (I wish I had a better solution to propose).

I could have used positive integer indexes for forward indexes and negative integer indexes for backward indexes but the problem was with the zero. Take help of this comment to understand it better. While reviewing #311 I got the idea of using stringlist_index_type indexes: forward and backward. Now, I need community's help in figuring out other problems. So feel free to leave your opinion.

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.

It seems that CMakeLists.txt and Makefile.manual have not been set up to compile your code, so maybe I should focus on understanding the issues you want to discuss, not grammatical typos.

src/stdlib_stringlist.f90 Outdated Show resolved Hide resolved
src/stdlib_stringlist.f90 Outdated Show resolved Hide resolved
src/stdlib_stringlist.f90 Outdated Show resolved Hide resolved
src/stdlib_stringlist.f90 Outdated Show resolved Hide resolved
@awvwgk awvwgk added the reviewers needed This patch requires extra eyes label Jul 31, 2021
@aman-godara

This comment has been minimized.

src/Makefile.manual Outdated Show resolved Hide resolved
@aman-godara
Copy link
Member Author

I have 2 questions:

1). Should the module be renamed to stringlist_type to be consistent with string_type?
2). Should to_current_idxn be made public?

@aman-godara
Copy link
Member Author

1). Should the module be renamed to stringlist_type to be consistent with string_type?
2). Should to_current_idxn be made public?

Let me rename module from stringlist to stringlist_type and keep to_current_idxn as private.

@aman-godara
Copy link
Member Author

All changes have been made.

Copy link
Member

@everythingfunctional everythingfunctional left a comment

Choose a reason for hiding this comment

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

I could probably nitpick some more, but honestly this is a pretty good first implementation. I also don't want to jeopardize you completing GSoC, because IMO, this is more than sufficient to pass. Thanks for the hard work you put in.

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

nice job. Thank you!
I approved it, pending some minor comments.

doc/specs/stdlib_stringlist_type.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stringlist_type.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stringlist_type.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stringlist_type.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stringlist_type.md Outdated Show resolved Hide resolved
doc/specs/stdlib_stringlist_type.md Outdated Show resolved Hide resolved
src/stdlib_stringlist_type.f90 Outdated Show resolved Hide resolved
src/stdlib_stringlist_type.f90 Show resolved Hide resolved
src/stdlib_stringlist_type.f90 Show resolved Hide resolved

!> Constructor to convert chararray to stringlist
!> Returns a new instance of type stringlist
pure function new_stringlist_carray( array )
Copy link
Member

Choose a reason for hiding this comment

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

Why is there not a constructor with array of type string_type?

Copy link
Member Author

@aman-godara aman-godara Aug 20, 2021

Choose a reason for hiding this comment

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

After size component was discarded it was causing ambiguous interfaces to provide support for string arrays. Which prompted me to remove it but I have added it again. Please have look at the commit 4b69e3d

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@jvdp1
Copy link
Member

jvdp1 commented Aug 20, 2021 via email

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @aman-godara for this work. Really useful.

@aman-godara
Copy link
Member Author

aman-godara commented Aug 23, 2021

Looks like to_string has been deleted or something from stdlib_ascii. It is one of the dependencies for stringlist.

@awvwgk
Copy link
Member

awvwgk commented Aug 23, 2021

to_string has been moved stdlib_strings with #444.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

Thanks @aman-godara and reviewers! Unless there are objections let's merge this tomorrow.

@milancurcic milancurcic merged commit 245926f into fortran-lang:master Aug 26, 2021
@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.

8 participants