On Mon, 14 Apr 2025 11:18:29 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> improved implementation of NullCoalescingPropertyBase
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042287057
