search

tc39 / ecmascript_simd

SIMD numeric type for EcmaScript
Other
544 stars 64 forks source link

Remove comments, regularize function definition and install. #258

Closed billbudge closed 9 years ago

johnmccutchan commented 9 years ago

@billbudge Your 'Refactor all the things' commit is too large for GitHub's preview. Can you please provide context and some snippets of that commit?

littledan commented 9 years ago

This looks great. @johnmccutchan , this patch is basically a rewrite, and you can look at it by viewing the whole ecmascript_simd.js file in the first patch (look at this: https://github.com/billbudge/ecmascript_simd/blob/c9f2aa634d6ade62cbec911872d57498ba6d7b58/src/ecmascript_simd.js ). This patch gets rid of basically all of the duplication in the SIMD polyfill by using a little bit of metaprogramming, which should make it a lot easier to add unsigned types and other things in the future. The smaller size makes it a more readable source for implementors.

@billbudge I think this would be cleaner if you squashed the last two fixes with the first patch, since they really do go together.

The only potentially objectionable part of this patch I see is that all the comments are gone. This is actually a change I suggested to Bill because it allows a lot less duplication, and the generated jsdoc wasn't really all that useful once there was both the spec and the MDN documentation. I think we should use the spec as the authoritative source of truth, with the MDN page as the readable introduction and no real need for a third set of written documentation in the form of code comments in the polyfill.

littledan commented 9 years ago

@ljharb What do you think?

littledan commented 9 years ago

Looks good to me.

ljharb commented 9 years ago

It's pretty hard to review because it's huge, but if it passes the tests and matches the spec, I'm sure it's fine.

billbudge commented 9 years ago

Sorry, in retrospect it could have been broken into easier to review patches. For example, I didn't realize I'd delete the comments until I was almost done, but I should have done that first.

johnmccutchan commented 9 years ago

@billbudge I blame GitHub's extremely limited diff tool. This isn't the first CL that I've seen GitHub choke on.