Request #48358 insertBeforeOffset() and insertAfterOffset() methods for ...#288
Request #48358 insertBeforeOffset() and insertAfterOffset() methods for ...#288MarkBaker wants to merge 5 commits intophp:PHP-5.5from
Conversation
…or SplDoublyLinkedList Adds new methods to insert a node in the middle of the dll object
…plDoublyLinkedList Modify code to use correct /* */ commenting and ensure that all indents are tabs rather than spaces
…plDoublyLinkedList Additional tests for function argument validation
|
Previous discussion on this: https://bugs.php.net/bug.php?id=48358 I agree that doubly linked lists need this ability to add elements in their inner content. The implementation seems OK (+- whitespaces). I however have some concern that with that patch, they interface of DLL becomes (even more) odd: To modify the content, you have: offsetSet, offsetUnset, and insertAfterOffset, insertBeforeOffset |
|
What are you suggesting: that they should be offsetInsertBefore() and offsetInsertAfter()? |
|
Well, I'm not sure, since offset* comes from ArrayAccess. But I would rather have some general-purpose discussion about interfaces of collections rather than adding one method at a time that we will not be able to rename in the future. |
|
I honestly fear that we are not reaching a conclusion on how to name the interfaces (and particularly as we cannot rename existing methods). Nevertheless I do think a linkedlist must have these methods similar methods to add elements to after an arbitrary poisiton. |
|
After an discussion with @colder, I think the best way to go is dropping insertAfter and adding a similar to Javas API. It behaves like insertBefore. The insertAfter can be easily written as a userland function using the add method. This way we keep things simple and our interface is similar to the Java LinkedList and C# LinkedList EDIT: NInja edit. Java add behaves like insertBefore not insertAfter.. |
👍 since |
|
The only argument I can think of for having both before and after versions is for after with an index of the last entry, effectively pushing a new value; but as push() is already available, this functionality is already available anyway. |
…plDoublyLinkedList Removed DLL insertAfterOffset() method; and renamed insertBeforeOffset() as add() Modified add() to handle a push if specified index = count of elements already in the DLL, while still adding before the specified index otherwise Adjusted tests accordingly to reflect the changes
|
Changes duly made as suggested to provide a single add() method |
|
This suggestion might be a bit iffy, but I'd quite like a |
|
I think if you want to push to the linked list, you would use push. It makes much more sense and is more readable than add(null, $value) |
|
I dont think we should handle NULL special. Anyway the patch looks good to me and if there are no objections I will merge it before beta1. |
|
Sounds good to me. 🐹 |
ext/spl/spl_dllist.c
Outdated
There was a problem hiding this comment.
Why does this have an (int) cast? index is declared as long, so the types should match.
There was a problem hiding this comment.
Mainly for the same reason that I wasn't certain why offsetUnset() also casts to int, while other methods like offsetExists() and offsetGet() don't: I wasn't sure if I was missing something elsewhere in the code, and preferred to err on the side of caution.... or is offsetUnset() also wrong in this cast?
There was a problem hiding this comment.
Seems wrong to me, I will fix offsetunset.
EDIT: Done
There was a problem hiding this comment.
In that case, I'll happily change add() and push again
Don't cast index to an int, leave it as a long
|
I squashed the branch to one commit and removed the newline changes and pushed it as 4257469. Thanks for the pull request. |
6 participants
...SplDoublyLinkedList
Adds new methods to insert a node in the middle of the dll object