Bug 237629

Created attachment 454175 [details]
Patch

Comment on attachment 454175 [details]
Patch

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

> Source/WebGPU/WGSL/CompilationMessage.h:57
> +using Warning = CompilationMessage;
> +using Error = CompilationMessage;

Cute.

> Source/WebGPU/WGSL/Parser.cpp:45
> +#define START_PARSE() \

Is this style of using #defines copied from anywhere?

> Source/WebGPU/WGSL/Parser.cpp:168
> +Expected<AST::ShaderModule, Error> parse(const String& wgsl)
> +{
> +    Lexer lexer(wgsl);
> +    Parser parser(lexer);
> +    
> +    return parser.parseShader();
> +}

Did you consider generating this code from a grammar file?

> Source/WebGPU/WGSL/Parser.cpp:511
> +    START_PARSE();

Is there a way to guarantee that parsing functions always start with a START_PARSE() macro?

Thanks for the review.

(In reply to Myles C. Maxfield from comment #2)
> Comment on attachment 454175 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454175&action=review
> 
> > Source/WebGPU/WGSL/CompilationMessage.h:57
> > +using Warning = CompilationMessage;
> > +using Error = CompilationMessage;
> 
> Cute.

Thanks, I found it makes function signatures more readable.

> > Source/WebGPU/WGSL/Parser.cpp:45
> > +#define START_PARSE() \
> 
> Is this style of using #defines copied from anywhere?

The exact macros are different, but both JavaScriptCore/parser/Parser.cpp and the old WHLSL parser (which I used as inspiration) use a ton of macros as well. Regular C++ is just not the most pleasant language for writing this kind of code.

> > Source/WebGPU/WGSL/Parser.cpp:168
> > +Expected<AST::ShaderModule, Error> parse(const String& wgsl)
> > +{
> > +    Lexer lexer(wgsl);
> > +    Parser parser(lexer);
> > +    
> > +    return parser.parseShader();
> > +}
> 
> Did you consider generating this code from a grammar file?

Yes, it would have been easier, but could have been an issue if the language later becomes more complicated. It would also have put a pretty low cap on the quality of the error messages that is achievable. As a result parser generators are rarely used for the major compilers for real-world languages.

> 
> > Source/WebGPU/WGSL/Parser.cpp:511
> > +    START_PARSE();
> 
> Is there a way to guarantee that parsing functions always start with a
> START_PARSE() macro?

Unfortunately I don't know of such a way. But forgetting the macro just gives you an error message as `_startOfElementPosition` is not defined.

Created attachment 454275 [details]
Patch for landing

Just fixed style nits.

Committed r291075 (248241@main): <https://commits.webkit.org/248241@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454275 [details].

<rdar://problem/90057433>