Bug 193389 – [WHLSL] Assorted cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WHLSL] Assorted cleanup

[Myles C. Maxfield]

Created attachment 359011 [details]
Patch

[Myles C. Maxfield]

Created attachment 359017 [details]
Patch

[Robin Morisset]

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

[Dean Jackson]

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 :)

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

(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.

[Robin Morisset]

(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.

[WebKit Commit Bot]

Comment on attachment 359017 [details]
Patch

Clearing flags on attachment: 359017

Committed r239930: <https://trac.webkit.org/changeset/239930>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47259060>