Bug 158648 - Air.js should have some documentation
Summary: Air.js should have some documentation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-10 17:19 PDT by Filip Pizlo
Modified: 2016-06-10 17:47 PDT (History)
4 users (show)

See Also:


Attachments
the patch (8.03 KB, patch)
2016-06-10 17:28 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (8.34 KB, patch)
2016-06-10 17:35 PDT, Filip Pizlo
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-06-10 17:19:20 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-06-10 17:28:41 PDT
Created attachment 281064 [details]
the patch
Comment 2 Filip Pizlo 2016-06-10 17:35:11 PDT
Created attachment 281066 [details]
the patch

Made minor fixes.
Comment 3 Keith Miller 2016-06-10 17:44:30 PDT
Comment on attachment 281066 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281066&action=review

r=me with some comments.

> PerformanceTests/Air.js/README.md:35
> +more readable. Unlike the original C++ Air, Air.js doesn't take advantage of
> +deep understanding of compilers to make the code easy to compile.

I think I would phrase this as "Unlike the original C++ Air, Air.js doesn't exploit a deep understanding of compilers to make the code easy to compile." The extra prepositions make the sentence seem clunky.

> PerformanceTests/Air.js/README.md:106
> +C++ Air and Air.js, and we assert that for each payload looks identical after

"that for each"  => each

> PerformanceTests/Air.js/README.md:109
> +allcoateStack, to help catch bugs from payload initialization. We have not

Nit: from => during
Comment 4 Filip Pizlo 2016-06-10 17:46:32 PDT
(In reply to comment #3)
> Comment on attachment 281066 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281066&action=review
> 
> r=me with some comments.
> 
> > PerformanceTests/Air.js/README.md:35
> > +more readable. Unlike the original C++ Air, Air.js doesn't take advantage of
> > +deep understanding of compilers to make the code easy to compile.
> 
> I think I would phrase this as "Unlike the original C++ Air, Air.js doesn't
> exploit a deep understanding of compilers to make the code easy to compile."
> The extra prepositions make the sentence seem clunky.

Yeah, that's much better.

> 
> > PerformanceTests/Air.js/README.md:106
> > +C++ Air and Air.js, and we assert that for each payload looks identical after
> 
> "that for each"  => each

Fixed.

> 
> > PerformanceTests/Air.js/README.md:109
> > +allcoateStack, to help catch bugs from payload initialization. We have not
> 
> Nit: from => during

Fixed.

Thanks for the review!
Comment 5 Filip Pizlo 2016-06-10 17:47:48 PDT
Landed in http://trac.webkit.org/changeset/201957