Bug 142759 – Compile character ranges targeting the same state as range check in the bytecode

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Compile character ranges targeting the same state as range check in the bytecode

[Benjamin Poulain]

Created attachment 248774 [details]
Patch

[Alex Christensen]

Comment on attachment 248774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248774&action=review

LGTM.  r- because there are a few small things to address.

> Source/WebCore/contentextensions/DFABytecode.h:47
> +    // The index to jump to if the values are equal (4 bytes).

"if the value is in the range"

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:117
> +    uint16_t lowValueKey;

I think either this should be a uint8_t or compileCheckForRange should take a uint16_t.  It's not easy to see where the type changes.
Also, I think rangeMin would be a better name for this variable.

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:118
> +    unsigned lowValueDestination = 0;

lowValueDestination -> rangeDestination?

> Source/WebCore/contentextensions/NFAToDFA.cpp:344
> +        if (!dfaNode.hasFallbackTransition && dfaNode.transitions.size() >= 126) {

This code is quite generalized.  I think it could be simplified:
If size==127, then everything goes to the fallback transition.  
If size==126, then we only have to find the one transition that is missing.

> Source/WebCore/contentextensions/NFAToDFA.cpp:362
> +            ASSERT_WITH_MESSAGE(bestTargetScore, "There should be a list one valid target since having transitions is a precondition to enter this path.");

"a list" -> "at least"
They sound the same with a Belgian accent :)

[Benjamin Poulain]

Created attachment 248793 [details]
Patch

[Benjamin Poulain]

Created attachment 248797 [details]
Patch

[Alex Christensen]

Comment on attachment 248797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248797&action=review

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:116
> +    bool hasrangeMin = false;

hasRangeMin

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:156
> +void DFABytecodeCompiler::compileCheckForRange(uint16_t lowValue, uint16_t highValue, unsigned destinationNodeIndex)

I think it would be good to assert lowValue and highValue are less than 128 here because we cast them to 8-bit integers.

> Source/WebCore/contentextensions/NFAToDFA.cpp:346
> +            && ((dfaNode.transitions.size() == 126 && !dfaNode.transitions.contains(0))
> +                || (dfaNode.transitions.size() == 127 && dfaNode.transitions.contains(0)))) {

I'm not sure I understand why the null character is a special case here.  Isn't it just another transition?

[Benjamin Poulain]

Committed r181663: <http://trac.webkit.org/changeset/181663>