Compile character ranges targeting the same state as range check in the bytecode
Created attachment 248774 [details] Patch
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 :)
Created attachment 248793 [details] Patch
Created attachment 248797 [details] Patch
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?
Committed r181663: <http://trac.webkit.org/changeset/181663>