Bug 193389 - [WHLSL] Assorted cleanup
Summary: [WHLSL] Assorted cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 193080
  Show dependency treegraph
 
Reported: 2019-01-13 17:58 PST by Myles C. Maxfield
Modified: 2019-01-14 11:22 PST (History)
7 users (show)

See Also:


Attachments
Patch (101.75 KB, patch)
2019-01-13 18:04 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (108.58 KB, patch)
2019-01-13 19:36 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-01-13 17:58:38 PST
[WHLSL] Assorted cleanup
Comment 1 Myles C. Maxfield 2019-01-13 18:04:51 PST
Created attachment 359011 [details]
Patch
Comment 2 Myles C. Maxfield 2019-01-13 19:36:20 PST
Created attachment 359017 [details]
Patch
Comment 3 Robin Morisset 2019-01-14 10:20:53 PST
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 4 Dean Jackson 2019-01-14 10:30:50 PST
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 :)
Comment 5 Myles C. Maxfield 2019-01-14 10:36:59 PST
(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.
Comment 6 Myles C. Maxfield 2019-01-14 10:38:03 PST
(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.
Comment 7 Robin Morisset 2019-01-14 11:04:39 PST
(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 8 WebKit Commit Bot 2019-01-14 11:21:57 PST
Comment on attachment 359017 [details]
Patch

Clearing flags on attachment: 359017

Committed r239930: <https://trac.webkit.org/changeset/239930>
Comment 9 WebKit Commit Bot 2019-01-14 11:21:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-01-14 11:22:29 PST
<rdar://problem/47259060>