[webkit-reviews] review granted: [Bug 233276] [WGSL] Implement enough of the lexer for the simplest shaders : [Attachment 451909] 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 233276: [WGSL] Implement enough of the lexer for the simplest shaders
https://bugs.webkit.org/show_bug.cgi?id=233276

Attachment 451909: Patch

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




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

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

> Source/WebGPU/ChangeLog:17
> +	   - space efficiency: we don't use an extra word of memory for the
variant's tag

https://github.com/akrzemi1/compact_optional can solve this.

> Source/WebGPU/Configurations/WGSLUnitTests.xcconfig:24
> +EXECUTABLE_PREFIX = lib;

I think I deleted this on purpose.
https://trac.webkit.org/changeset/288608/webkit

> Source/WebGPU/WGSL/Lexer.cpp:32
> +Token Lexer<T>::lex()

Why not generate this?

> Source/WebGPU/WGSL/Lexer.cpp:44
> +	   return makeToken(TokenType::ParenLeft);

Can you add a Token(TokenType) implicit constructor to avoid all these
makeToken() calls?

> Source/WebGPU/WGSL/Lexer.cpp:197
> +	   if (isIdentifierStart(m_current)) {

It might be worth doing the same kind of range check for 1-9 instead of having
a bunch of case statements.

> Source/WebGPU/WGSL/Lexer.cpp:372
> +    case ' ':
> +    case '\t':
> +    case '\n':
> +    case '\v':
> +    case '\f':
> +    case '\r':

We have more-friendly names for these in wtf/characternames.h

> Source/WebGPU/WGSL/Lexer.cpp:435
> +template <>
> +ALWAYS_INLINE bool Lexer<UChar>::isIdentifierStart(UChar ch)
> +{
> +    // WGSL does not currently support any non-ASCII characters in
identifiers
> +    return Lexer<LChar>::isIdentifierStart(static_cast<LChar>(ch));
> +}

Why specialize if the implementation is the same?

> Source/WebGPU/WGSL/Lexer.h:39
> +	   if constexpr (std::is_same<T, LChar>::value) {

I'd suggest either using template specialization or using `if constexpr` so the
`else` block at the bottom can be a static_assert instead of a release_assert.

> Source/WebGPU/WGSL/SourceSpan.h:33
> +    unsigned m_line;
> +    unsigned m_lineOffset;
> +    unsigned m_offset;

Why not have at least one of these fields be derived? That way, we don't have
to track as much in the common case (the compile succeeds)

> Source/WebGPU/WGSL/Token.cpp:31
> +String toString(TokenType type)

Again, wish this could be generated.

> Source/WebGPU/WGSL/Token.h:37
> +    // - space efficiency: we don't use an extra word of memory for the
variant's tag

See changelog.

> Source/WebGPU/WGSL/Token.h:38
> +    // - ease of use: everywhere that we check for a given TokenType, we
would have to first check that the Token is not nullopt, and then check the
TokenType.

I think this is slightly misstated - it's that users will already have to check
the token type, so if we fold the optional into the token type, users get 2
checks for the price of one. Easier to use, and slightly more performant, and
still just as type-safe as the alternative.

>> Source/WebGPU/WGSLUnitTests/WGSLLexerTests.mm:31
>> + at interface WGSLLexerTests : XCTestCase
> 
> Seems weird to use XCTest here when we use GTest for the unit tests elsewhere
in the code base. Given the cross platform history of WebKit, I don't think
using a single platform testing tool likely makes long term sense.

The integration point of WebGPU is above this. Non-apple ports want to use Dawn
or wgpu instead of WebGPU.framework, so they wouldn't ever run these tests.
WebGPU.framework, and this compiler inside it, is already Metal-specific.