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

pop, drop & get with basic range feature for stringlist #514

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

aman-godara
Copy link
Member

@aman-godara aman-godara commented Sep 6, 2021

Delete function to stdlib_stringlist

Tasks:

@aman-godara
Copy link
Member Author

delete function vs remove function?

delete functions returns the deleted element.
remove function does same job as to delete function but doesn't return the deleted element.

Any suggestions on better naming convention? Should I rather call delete as pop and think of a better name for remove?

@epagone
Copy link

epagone commented Sep 7, 2021

This is highly subjective but FWIW, I would prefer pop for the functions that return the deleted element, simply because I imagine that if I "pop" something out I still have it with me.

For the function that does not return the deleted element I am fine with both remove or delete, with a slight preference for delete.

Did you have any time to see what are the choices of other languages for similar functions?

@aman-godara
Copy link
Member Author

Python uses pop (returns deleted item) and remove (no return)
Java doesn't have any function equivalent of pop (or atleast I couldn't find one), it has remove function (no return).

pop function is used whenever deleted item is returned, otherwise remove and delete functions both are used by programming languages. Though, I could find only one occurrence where delete was used to remove elements.
Also, some languages uses pop() (with no arguments) as a way to allow a user to make a list behave like a stack or a queue.

@arjenmarkus
Copy link
Member

arjenmarkus commented Sep 7, 2021 via email

@awvwgk
Copy link
Member

awvwgk commented Sep 8, 2021

I think pop is a reasonable name to a procedure to remove and return the removed element. For only deleting elements, delete, remove or drop might be suitable. The names delete and destroy could be associated with a deconstructor of the entire list.

@epagone
Copy link

epagone commented Sep 10, 2021

Based on my experience with past discussions, we are trying to provide a familiar interface to the user of stdlib following well-established conventions from other languages (like python, for example). @arjenmarkus suggests more "Fortranic" names (that I like too) but they might be inconsistent with other parts of stdlib.

Based on the other suggestions, my preference is

  • pop: removes and returns the removed element
  • drop: removes an element without returning it
  • destroy: de-constructs the list and frees the memory

I do not have strong opinions on this, though.

@aman-godara
Copy link
Member Author

I agree with destroy (please note that we have a function named clear in stdlib_stringlist which resets the list and NOT destroys it). Checkout this example from python to understand the difference:

image

@aman-godara
Copy link
Member Author

aman-godara commented Sep 12, 2021

I added a basic range feature to pop and drop which will remove all strings at indexes in the interval [first, last].
Out of bounds indexes in this interval will be ignored. There is no concept of stride currently in this range feature.

We can discuss the behaviour of non-basic range feature for pop and drop.

I think atleast a basic range feature should be added to get function as well, it will also improve the way pop_engine function is written.

@epagone
Copy link

epagone commented Sep 13, 2021

Thank you.

I personally don't like that out of bounds indexes are ignored: I'd like to be notified (at least with a warning, if not by an error) in these cases. Based on my experience, out of bounds indexes are the consequence of upstream bugs that would be difficult to identify, if ignored. However, I believe that a choice in this sense might have been already made during the initial implementation of stringlist for other methods.

I think atleast a basic range feature should be added to get function as well, it will also improve the way pop_engine function is written.

Good point, I agree.

@epagone
Copy link

epagone commented Sep 17, 2021

I am under the impression that adding the get function (although, as I said, it's a good idea) is out of scope for this PR. Am I right?

@aman-godara
Copy link
Member Author

I am under the impression that adding the get function (although, as I said, it's a good idea) is out of scope for this PR. Am I right?

Yeah, that's right. Let me rename PR.

@aman-godara aman-godara changed the title Delete stringlist & move subroutine pop, drop & get with basic range feature for stringlist Sep 17, 2021
@awvwgk awvwgk added reviewers needed This patch requires extra eyes topic: strings String processing labels Sep 18, 2021
@aman-godara
Copy link
Member Author

This PR is blocked by: #552

get_engine func takes an integer stride_vector which decides the number
of strides to take between indexes.

get_range_idx func takes stride of type stringlist_index_type:
   fidx(+3) means takes +3 as stride (jump 3 indexes to right)
   fidx(-3) means takes -3 as stride (jump 3 indexes to left)

   bidx(+3) means takes -3 as stride (jump 3 indexes to left)
   bidx(-3) means takes +3 as stride (jump 3 indexes to right)
get_impl comes in the middle of get_engine and get_idx routines

This allows to get i-th element inside the module using integer indexes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewers needed This patch requires extra eyes topic: strings String processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants