On Mon, 14 Apr 2025 14:35:43 GMT, Michael Strauß <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/TokenStream.java
>> line 31:
>>
>>> 29: import java.util.function.Predicate;
>>> 30:
>>> 31: public final class TokenStream {
>>
>> I think the naming of the methods in this class leaves something to be
>> desired.
>>
>> - `Token consume()` **OK**
>> - `Token consume(Predicate)` -> `consumeIf` or `consumeIfMatches`
>> Makes it clearer, as `consume` seems to imply something is always
>> consumed (ie. it still skips one token if the predicate didn't match
>> returning `null`).
>> - `Token peek()` **OK**
>> - `reconsume` -> `unconsume`, `undoConsume`, `undo`, `previous`,
>> `resetToPrevious`, `decrementIndex`
>> Nothing is consumed which reconsume seems to imply, instead it moves the
>> index back one place so the next call to `consume` may indeed reconsume a
>> token; reconsume as-is would do nothing (and if it returned anything it
>> would be same the as `current`.
>> - `boolean peekMany(Predicate...)` -> `matches`
>> It doesn't work like `peek`. `peekMany` would imply it returns a
>> `List<Token>`; it also doesn't convey that it returns a `boolean`.
>> - `reset(int)` -> `setIndex`
>> This seems to be similar to what say `InputStream` provides, but
>> `InputStream` hides the `index` parameter so you can't set it to some
>> arbitrary value (like skipping ahead). If you want to mimic this pattern,
>> I'd suggest removing the parameter and providing a `mark` method (or make it
>> non-public).
>
> I renamed `consume(Predicate)` to `consumeIf(Predicate)`, and
> `peekMany(Predicate...)` to `matches(Predicate...)`.
>
> `index()` and `reset()` don't need to be public, so I've removed that (also I
> don't want to mimic the problematic mark/reset pattern of `InputStream`).
>
> `reconsume` is [CSS
> lingo](https://www.w3.org/TR/css-syntax-3/#tokenizer-definitions), I'm
> keeping that as-is.
`reconsume` -> `resetToPrevious` + mention "reconsume" in javadoc?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2072177991
