[webkit-reviews] review granted: [Bug 199520] [WHLSL] Optimize the lexer : [Attachment 373518] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 199520: [WHLSL] Optimize the lexer
https://bugs.webkit.org/show_bug.cgi?id=199520

Attachment 373518: Patch

https://bugs.webkit.org/attachment.cgi?id=373518&action=review




--- Comment #4 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 373518
  --> https://bugs.webkit.org/attachment.cgi?id=373518
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:253
> +	   return "LEXING_ERROR";

Why not put a space in it? This is just used for error messages.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:500
> +    if (isFullyConsumed())

isFullyConsumed calls peek() which I think calls this. Doesn't this have a
chance of infinite looping?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:519
> +//	     which means we'll never see a false match. If we see a BMP code
unit, we
> +//	     really have a BMP code point.

I know you didn't do this, but can we fix the indentation?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:670
> +    while (auto result = digit(offset))

Cool.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:-759
> -    if (auto result = string("operator&.", offset))

I don't see where this moved to. How do we lex operator&.foo now?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:202
> +	   Token ringBuffer[2];

Looks like your patch hasn't been rebased. There's now a m_offsetRingBuffer[2].

Does perf improve if we delete m_offsetRingBuffer[2]? It isn't actually used;
it's just a convenience. If perf improves, perhaps we could put it behind a
DEBUG preprocessor macro.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:195
> +template <Lexer::Token::Type... types>
> +Optional<Lexer::Token> Parser::tryTypes()

Does this cause code bloat? If so, is the performance gain worth it?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:212
> +template <Lexer::Token::Type... types>
> +auto Parser::consumeTypes() -> Expected<Lexer::Token, Error>

ditto