Add a (copy-)efficient Persistent Stack Implementation - #65
Conversation
…hecker. This PersistentLinkedStack comes with an iterator and serialization proxy. It is copy efficient and allows O(1) push/pop.
| * @param <T> The type of values. | ||
| */ | ||
| @Immutable(containerOf = "T") | ||
| public interface PersistentStack<T> extends Iterable<T>, Serializable { |
There was a problem hiding this comment.
I would think it is better to not extend Iterable in PersistentStack. I personally find it always confusing what the iteration order of a stack is, and would never be sure without looking it up, also because of inconsistency between related classes: For PersistentStack, the only meaningful iteration order is top-down (last element pushed is first). The (de facto deprecated and unused, but still existing) java.util.Stack has a bottom-up iteration order, because it is based on a list and pushes to the end. The standard stack in Java (java.util.Deque) iterates top-down if you use push(), but bottom-up if you use add().
We can instead provide a O(1) method asReverseIterable or so that returns an Iterable (or likely even a SequencedCollection if not a List) view. The name would make it clear that the iteration order is the reverse of the addition order. Or maybe asTopDownIterable() even?
Additionally we can provide a O(n) copyToList() that returns the elements in addition (bottom-up) order.
| * <p>Implementations support standard Java Object Serialization. Serialization succeeds only if | ||
| * each contained value and its serialized object graph are serializable at runtime; otherwise, | ||
| * serialization fails according to the standard rules, for example with {@link | ||
| * java.io.NotSerializableException}. | ||
| * | ||
| * <p>This serialization contract applies to conforming Java SE runtimes. GraalVM in JVM mode uses | ||
| * the same semantics, while GraalVM Native Image may require explicit serialization metadata or | ||
| * configuration. Support in non-Java-SE environments, such as Android or GWT, is not guaranteed. | ||
| * Deserialization may also be rejected by configured {@link java.io.ObjectInputFilter} policies, | ||
| * and portability of serialized data depends on the serialized forms of contained values. | ||
| * | ||
| * <p>After a stack reference has been made visible to other threads through synchronization, a | ||
| * {@code volatile} field, or a concurrency utility, its immutable structure may be accessed | ||
| * concurrently. Such coordination is still required to publish or update a shared reference to a | ||
| * stack version, and compound updates require synchronization or an atomic operation. No | ||
| * thread-safety guarantee is made for iterator instances. | ||
| * | ||
| * <p>Values are stored by reference: they are not copied or made immutable or thread-safe. Changes | ||
| * to mutable values can affect equality and hash codes. Operations that depend on values also | ||
| * depend on their thread safety. |
There was a problem hiding this comment.
Sure, this is all true, but it is also the same for all such collections and pretty standard. I would shorten to just say something like "Serializable if all elements are serializable" and "thread safe including its collection views, but no guarantees for the iterator".
| 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.
| * @param <T> The type of values. | ||
| */ | ||
| @Immutable(containerOf = "T") | ||
| public interface PersistentStack<T> extends Iterable<T>, Serializable { |
There was a problem hiding this comment.
What is missing in this interface is a definition of equals and hashCode. Typical data structures define these precisely such that even different implementations of the same interface are guaranteed to compare equal. The current equals and hashCode implementations would not achieve this, for example.
| } | ||
| } | ||
|
|
||
| private static final class StackIterator<T> extends UnmodifiableIterator<T> { |
|
|
||
| @Override | ||
| public boolean hasNext() { | ||
| return stack != null && !stack.isEmpty(); |
There was a problem hiding this comment.
Only one of these conditions should be needed. If is confusing if there are two possible different states of the iterator that both mean "end of iteration".
| * brackets: {@code [top, ..., bottom]}. The empty stack is represented as {@code []}. | ||
| */ | ||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
Can be implemented trivially using Joiner or Collectors.joining().
| * @param <T> the type of values | ||
| */ | ||
| @Immutable(containerOf = "T") | ||
| public final class PersistentLinkedStack<T> implements PersistentStack<T> { |
There was a problem hiding this comment.
We also want factory methods, which then can be used for deserialization.
And for example a Collector implementation is also missing.
|
We can consider adding some bulk operations, for example a method to get the bottom n elements, similar to |
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.