[webkit-reviews] review denied: [Bug 175925] Initial reference implementation scaffolding for WebGPU Shading Language prototype : [Attachment 319284] the patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 30 00:07:09 PDT 2017
Keith Miller <keith_miller at apple.com> has denied review:
Bug 175925: Initial reference implementation scaffolding for WebGPU Shading
Language prototype
https://bugs.webkit.org/show_bug.cgi?id=175925
Attachment 319284: the patch
https://bugs.webkit.org/attachment.cgi?id=319284&action=review
--- Comment #30 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 319284
--> https://bugs.webkit.org/attachment.cgi?id=319284
the patch
View in context: https://bugs.webkit.org/attachment.cgi?id=319284&action=review
It's Keith's beddy-bye time, I'm tentatively r-ing based on my comments. I'll
read more details tomorrow to make sure I understand correctly.
>>> Tools/ChangeLog:3
>>> + Initial reference implementation scaffolding for ArrayLang
>>
>> WebGPU Shading Language
>
> Let’s come up with a shorter name, or use ArrayLang in the initial commit.
Renaming is easy.
Why not WebSL? Are there any shader languages that wouldn't be for the GPU?
>>> Tools/ChangeLog:24
>>> + Each variable behaves as if it was declared "static", with one
copy per type instantiation.
>>
>> Using C++'s notion of "static" variables
>
> This reference implementation will run in a browser in the sense that we will
make it a page you can navigate to.
I don't like this semantics, unless it's a compile error to use an
uninitialized value. In fact, I'll r- on this point alone. e.g.
int foo() {
int a;
return a++;
}
would return a different value on each invocation.
>> Tools/ArrayLangRI/ALSyntaxError.js:23
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>
> The trailing whitespace in these copyright comments is like a small virus
that spreads throughout our codebase =)
Indeed, pizlo needs to install https://github.com/lewang/ws-butler. I
recommended it to him previously but he chose to ignore me :(
>>> Tools/ArrayLangRI/AddressSpace.js:27
>>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"];
>>
>> In a browser, I think the the lexical nature of the `const` would prevent
this constant from being used outside of this file. Same with the cases of just
`class Foo { ... };`. In your case I think it all works in because All.js just
inline `load(...)`s all the individual files. You don't need to change, just
something to consider if you want to try running this in a browser eventually.
>
> We don't want to run this in a browser eventually. This is a
proof-of-concept.
Unless I have misunderstood you Myles, I think we do want this to run in a
browser. My understanding is this would be more or less a reference
implementation.
More information about the webkit-reviews
mailing list