1210291 - Streamline StreamingLexer's handling of terminal states

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

As a step on the way to getting rid of StreamLexer's need for state values we need to change SUCCESS and FAILURE to be their own separate things.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Streamline StreamingLexer's handling of terminal states

This patch introduces TerminalState and changes LexerTransition::mNextState to
be a Variant<State, TerminalState>. This means that SUCCESS and FAILURE no
longer need to be part of State.

Some things to note:

- This simplifies the handling of Lex()'s return value, which is nice.

- The patch splits Terminate() into TerminateSuccess() and TerminateFailure().
  You have to specify the State template type at every callsite, unfortunately.

- |const State& aNextState| wouldn't work for the first arg to
  LexerTransition's ctor due to errors in Variant construction that I didn't
  understand. I had to change it to |State aNextState|.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: tweak LexerTransition

Here's a slightly different way of structuring LexerTransition. It's nicer in
that the fields which are only relevant for non-terminal states are now
contained within the Variant. But it's also more verbose. I could go either way
on whether to do it like this or not.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Streamline StreamingLexer's handling of terminal states

Updated version of part 1 to account for the recent changes to nsBMPDecoder.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8675959 [details] [diff] [review]
Streamline StreamingLexer's handling of terminal states

Review of attachment 8675959 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/StreamingLexer.h
@@ +34,4 @@
>  {
> +  SUCCESS,
> +  FAILURE,
> +  INCOMPLETE

INCOMPLETE isn't a terminal state. I think we're better off continuing to return a Maybe from Lex().

::: image/decoders/nsICODecoder.cpp
@@ +230,5 @@
>  nsICODecoder::ReadHeader(const char* aData)
>  {
>    // If the third byte is 1, this is an icon. If 2, a cursor.
>    if ((aData[2] != 1) && (aData[2] != 2)) {
> +    return Transition::TerminateFailure<ICOState>();

The verbosity of specifying the state type here isn't ideal. Why don't we just have Transition::TerminateFailure() / Transition::TerminateSuccess() return a TerminalState, and provide an implicit conversion from TerminalState to LexerTransition<T> for any T? (You already did, actually, since you didn't mark the new constructor you added explicit.) Such a conversion is always valid, since T doesn't matter and LexerTransition has no other state in the terminal case.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Streamline StreamingLexer's handling of terminal states

New version which makes the two requested modifications.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: tweak LexerTransition

New version of the second patch; see comment 2 for what's happening here.

Comment 7

[Nicholas Nethercote [inactive]]

Seth: 13 day review ping.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8679868 [details] [diff] [review]
Streamline StreamingLexer's handling of terminal states

Review of attachment 8679868 [details] [diff] [review]:
-----------------------------------------------------------------

Really nice, Nick! r+, with some nits below.

::: image/StreamingLexer.h
@@ +146,5 @@
>                                    BufferingStrategy::BUFFERED);
>    }
>  
>    /**
> +   * Terminate lexing, ending up in terminal state FAILURE. (The implicit

You have FAILURE in the comment for TerminateSuccess() and SUCCESS in the comment for TerminateFailure(). Should switch those. =)

@@ +260,5 @@
>        size_t toRead = std::min(mToReadUnbuffered, aLength);
>  
> +      // Call aFunc with the unbuffered state to indicate that we're in the
> +      // middle of an unbuffered read. We enforce that any state transition
> +      // passed back to us is either a terminal states or takes us back to the

Nit: This was my typo, but s/terminal states/terminal state.

::: image/decoders/nsBMPDecoder.cpp
@@ +445,4 @@
>        }
>      });
>  
> +  if (terminalState && *terminalState == TerminalState::FAILURE) {

This can be written as just |if (terminalState == Some(TerminalState::FAILURE))|.

::: image/decoders/nsICODecoder.cpp
@@ +644,4 @@
>        }
>      });
>  
> +  if (terminalState && *terminalState == TerminalState::FAILURE) {

See above.

::: image/decoders/nsIconDecoder.cpp
@@ +57,4 @@
>        }
>      });
>  
> +  if (terminalState && *terminalState == TerminalState::FAILURE) {

See above.

::: image/test/gtest/TestStreamingLexer.cpp
@@ +88,5 @@
>    char data[9] = { 1, 2, 3, 1, 2, 3, 1, 2, 3 };
>  
>    // Test delivering all the data at once.
> +  Maybe<TerminalState> result = lexer.Lex(data, sizeof(data), DoLex);
> +  EXPECT_TRUE(result.isSome() && *result == TerminalState::SUCCESS);

I'd prefer not to make this change (and same for the other places where it appears); this way we can immediately tell the difference in the logs between the lexer *not finishing at all* (indicated by result.isNothing()) and the lexer *finishing, but failing* (indicated by result != Some(TerminalState::SUCCESS).

I really should've written |EXPECT_EQ(Some(TestState::SUCCESS), result);|, though. I'm not sure why I didn't; maybe it was a slipup, or maybe there's some issue with GTest's macro magic.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8679869 [details] [diff] [review]
tweak LexerTransition

Review of attachment 8679869 [details] [diff] [review]:
-----------------------------------------------------------------

I like it. Should make it harder to introduce bugs in LexerTransition when further maintenance happens down the line. (The only negative is the verbosity, as you say, but that's C++ for you.)

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/793133ff5233a8e2cd0a6864b9984f5a9bb31c05
Bug 1210291 - Streamline StreamingLexer's handling of terminal states. r=seth.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/793133ff5233