[WHLSL] Assorted cleanup
Created attachment 359011 [details] Patch
Created attachment 359017 [details] Patch
Comment on attachment 359017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359017&action=review > Source/WebCore/ChangeLog:16 > + - EnumTypes have to have non-null base types. The parser will make sure this is always true. I have not seen this last rule when looking through the patch. It seems a bit weird to me to have such semantic rule verified by the parser. The patch LGTM otherwise
Comment on attachment 359017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359017&action=review rs=me assuming robin's comment is addressed. > Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:60 > + UnnamedType* type() { return m_type ? &*m_type : nullptr; } Is this the syntax we usually use for an Optional<UniqueRef<>> ? It looks weird because at first glance it appears to do nothing :)
(In reply to Robin Morisset from comment #3) > Comment on attachment 359017 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359017&action=review > > > Source/WebCore/ChangeLog:16 > > + - EnumTypes have to have non-null base types. The parser will make sure this is always true. > > I have not seen this last rule when looking through the patch. > It seems a bit weird to me to have such semantic rule verified by the parser. > The patch LGTM otherwise It’s not verified by the parser; the parser just sets the base type to be a reference to “int” if it isn’t present. This is what the reference compiler does.
(In reply to Dean Jackson from comment #4) > Comment on attachment 359017 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359017&action=review > > rs=me assuming robin's comment is addressed. > > > Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:60 > > + UnnamedType* type() { return m_type ? &*m_type : nullptr; } > > Is this the syntax we usually use for an Optional<UniqueRef<>> ? It looks > weird because at first glance it appears to do nothing :) I don’t know any better way to get a pointer to the thing inside it.
(In reply to Myles C. Maxfield from comment #5) > (In reply to Robin Morisset from comment #3) > > Comment on attachment 359017 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=359017&action=review > > > > > Source/WebCore/ChangeLog:16 > > > + - EnumTypes have to have non-null base types. The parser will make sure this is always true. > > > > I have not seen this last rule when looking through the patch. > > It seems a bit weird to me to have such semantic rule verified by the parser. > > The patch LGTM otherwise > > It’s not verified by the parser; the parser just sets the base type to be a > reference to “int” if it isn’t present. This is what the reference compiler > does. OK, I misunderstood your comment as saying that the parser would reject enums who are given an explicit base type by the programmer, when that explicit base type is "null". Which sounded a bit weird, but what you are saying is perfectly reasonable.
Comment on attachment 359017 [details] Patch Clearing flags on attachment: 359017 Committed r239930: <https://trac.webkit.org/changeset/239930>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47259060>