Closed Bug 1210291 Opened 4 years ago Closed 4 years ago

Streamline StreamingLexer's handling of terminal states

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files, 3 obsolete files)

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.
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|.
Attachment #8668269 - Flags: review?(seth)
Attached patch tweak LexerTransition (obsolete) — Splinter Review
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.
Attachment #8668270 - Flags: review?(seth)
Updated version of part 1 to account for the recent changes to nsBMPDecoder.
Attachment #8675959 - Flags: review?(seth)
Attachment #8668269 - Attachment is obsolete: true
Attachment #8668269 - Flags: review?(seth)
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.
New version which makes the two requested modifications.
Attachment #8679868 - Flags: review?(seth)
Attachment #8675959 - Attachment is obsolete: true
Attachment #8675959 - Flags: review?(seth)
New version of the second patch; see comment 2 for what's happening here.
Attachment #8679869 - Flags: review?(seth)
Attachment #8668270 - Attachment is obsolete: true
Attachment #8668270 - Flags: review?(seth)
Seth: 13 day review ping.
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.
Attachment #8679868 - Flags: review?(seth) → review+
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.)
Attachment #8679869 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/793133ff5233
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.