Remove unnecessary variable 'state' in LexerImpl by using Stack

[yaboong]

In my opinion, the 'state' variable in LexerImpl seems unnecessary. You can remove them by using Stack data structure to check the current state. (Replaced LinkedList to Stack to make it clear that the 'states' is managed by Stack)

All the test cases have passed.

[zimmi]

java.util.Stack is effectively deprecated in favor of Deque, here is what the javadoc has to say:

A more complete and consistent set of LIFO stack operations is provided by the Deque interface and its implementations, which should be used in preference to this class. For example:
Deque<Integer> stack = new ArrayDeque<Integer>();

[yaboong]

@zimmi
Thanks for letting me know.
However, the 'state' variable still seems unnecessary for me because you can use the peek() method to check the current state. Is it okay to create another PR to remove the 'state' variable from the LexerImpl?

[zimmi]

I'm not a maintainer of this repo, just a happy user of pebble. :)
That said, your change looks fine to me. Just replace the Stack with ArrayDeque, the previous LinkedList is probably also less efficient. Most methods of Stack are needlessly synchronized, that's one of the reasons it shouldn't be used anymore. The javadoc of ArrayDequesays:

This class is likely to be faster than Stack when used as a stack, and faster than LinkedList when used as a queue.

I'd just update this PR, but you might want to wait for one of the maintainers to make the call.

[yaboong]

@zimmi
Thank you for your feedback. As you said, I think I should wait until the manager confirms this PR.

I've checked the javadoc you gave me and it says,

They (Array deques) are not thread-safe; in the absence of external synchronization, they do not support concurrent access by multiple threads.

Because ArrayDeque is not thread-safe, and the pebble needs to support complete thread safety, I think it is dangerous to change all the Stack to Deque just because of the performance issue.

Anyways, your comment helped me to better understand Java. Thanks :)

[zimmi]

Instances of LexerImpl are never called from multiple threads, each thread creates its own instance. So the synchronization is useless. That aside, just using thread safe data structures is not enough to achieve thread safety of a whole program. See this SO question for the concrete case of Vector / Stack.

[ebussieres]

Should you use Deque instead of Stack ?

[yaboong]

This could be debatable. Deque could be better if you're looking at performance, but Stack is better in terms of semantics.

Deque can be used like Stack at times, and sometimes like Queue but I think this is a double edged sword. Stack, on the other hand, is just Stack. A Stack cannot be used just like a Queue even if it extends Vector.

I think it's a matter of choice but I prefer to use Stack.

[slandelle]

IMHO, no, choice is not debatable: Stack is obsolete and Dequeue’s performance is better.

[yaboong]

Alright then is it okay to close this PR and make another PR to change all the Stack in LexerImpl to Deque?

[slandelle]

Why not edit this one? You just need to update your fork.

[ebussieres]

No need to close this PR. Just commit and push to your branch and the PR will bu updated

[yaboong]

It's done. Thanks.