Repository navigation
Conversation
…hecker. This PersistentLinkedStack comes with an iterator and serialization proxy. It is copy efficient and allows O(1) push/pop.
| import java.util.NoSuchElementException; | ||
| import org.junit.Test; | ||
|
|
||
| public class PersistentLinkedStackTest { |
There was a problem hiding this comment.
Please add tests based on Guava's testlib like we have for PathCopyingPersistentTreeMap. These are much more comprehensive than what we would think of.
There was a problem hiding this comment.
I tried following it and refactored the tests. Could you please take a look whether this is sufficient?
There was a problem hiding this comment.
I don't think testing the result of copyToList() is useful. This would test the creation of the stack, the copyToList() call and then all the features of the returned list implementation and its views. But it will not test any of the other PersistentLinkedStack features and methods.
|
We can consider adding some bulk operations, for example a method to get the bottom n elements, similar to |
…le() and copyToList(), as well as implementations and updates of calls relying on iterable
…ents, and copyOf() impl, as well as toPersistentLinkedStack() impl + updates to readResolve() based on the new methods
…ttom of the PersistentStack
I added a implementation for this. |
…istentStack impl for hashcode and equals tests
| @SuppressWarnings("NoFunctionalReturnType") | ||
| public static <T> Collector<T, ?, PersistentLinkedStack<T>> toPersistentLinkedStack() { | ||
| return Collectors.collectingAndThen( | ||
| ImmutableList.<T>toImmutableList(), PersistentLinkedStack::copyOf); |
There was a problem hiding this comment.
I don't think we want to first copy everything into a list and then into a stack. That is what users could easily do without our collector.
There was a problem hiding this comment.
I improved it some more, but we can't fully eliminate temporary buffering other stacks that are combined.
|
|
||
| @Override | ||
| public ImmutableList<T> copyToList() { | ||
| return ImmutableList.copyOf(asTopDownIterable()).reverse(); |
There was a problem hiding this comment.
We could use a Builder with the known size to make this more efficient.
There was a problem hiding this comment.
Actually, we should keep the code here as is, but it is a good argument for asTopDownIterable() to return a Collection, because then copyof() will automatically use the correct size.
| * Returns an unmodifiable top-to-bottom view in O(1) time. Each iterator traverses this stack | ||
| * version independently. | ||
| */ | ||
| Iterable<T> asTopDownIterable(); |
There was a problem hiding this comment.
If we can return a Collection here it could be more convenient for callers. In principle SequencedCollection would be even better semantically, but it only adds reversed(), which we can hardly support, and some mutation methods. So there is little point in it from the API point of view.
There was a problem hiding this comment.
We now return a view that is a unmodifiable Collection.
| @Var PersistentLinkedStack<Object> stack = PersistentLinkedStack.of(); | ||
| // The serialized order is top-to-bottom; push in the opposite direction. | ||
| for (@Var int index = serializedValues.length - 1; index >= 0; index--) { | ||
| @Nullable Object value = serializedValues[index]; | ||
| if (value == null) { | ||
| throw new InvalidObjectException("Stack values must not contain null"); | ||
| } | ||
| stack = stack.pushAndCopy(value); | ||
| } | ||
| return stack; |
There was a problem hiding this comment.
We can replaced this with something based on copyOf(Arrays.asList(values).reversed()).
There was a problem hiding this comment.
Done. I did not do it in one line so that comments explain what's going on more easily (and a TODO for Java 21).
| import java.util.NoSuchElementException; | ||
| import org.junit.Test; | ||
|
|
||
| public class PersistentLinkedStackTest { |
There was a problem hiding this comment.
I don't think testing the result of copyToList() is useful. This would test the creation of the stack, the copyToList() call and then all the features of the returned list implementation and its views. But it will not test any of the other PersistentLinkedStack features and methods.
…ering all input stacks into a list, instead directly construct a new stack from them, unless combining 2 LinkedStacks, since then we need to buffer the elements of one stack
… simplifying its readResolve implementation
…stack, not the list copy of it, and extend with more cases that were not covered
This branch has not been deployed
2 participants
We have a
PersistentStackimplementation in CPAchecker, but not in this library. It makes sense to move it here.While doing so, the following was added:
PersistentStack.PersistentLinkedStackTest.PersistentLinkedStackimplementation that provides serialization through a proxy and an iterator + proper documentation.Note: the current state is a hybrid of our
PersistentStackin CPAchecker andPersistentLinkedListin this repo, but extended using LLM generated code. I reviewed the changes done by the LLM before opening this PR. I am unsure about the serialization though, so input would be appreciated.