Bug 122081 - FTL: refactor compileAdd and compileArithSub into one function.
Summary: FTL: refactor compileAdd and compileArithSub into one function.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-29 18:45 PDT by Nadav Rotem
Modified: 2013-09-29 23:31 PDT (History)
1 user (show)

See Also:


Attachments
Patch (5.48 KB, patch)
2013-09-29 18:45 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2013-09-29 20:58 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2013-09-29 22:09 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nadav Rotem 2013-09-29 18:45:00 PDT
FTL: refactor compileAdd and compileArithSub into one function.
Comment 1 Nadav Rotem 2013-09-29 18:45:14 PDT
Created attachment 212941 [details]
Patch
Comment 2 Geoffrey Garen 2013-09-29 19:02:06 PDT
Comment on attachment 212941 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:285
>          case ValueAdd:
> -            compileAdd();
> +            compileAddSub(false /* this is an add */);
>              break;
>          case ArithSub:
> -            compileArithSub();
> +            compileAddSub(true /* this is a sub */);

WebKit style for booleans that are "true" or "false" constants at the call site is to use named enum values. Something like:

enum AddOrSub { Add, Sub };
compileAddOrSub(Add);
compileAddOrSub(Sub);

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:696
> +            LValue C1 =lowDouble(m_node->child1());
> +            LValue C2 =lowDouble(m_node->child2());

Need spaces after the "="s.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:707
> -    
> +

Please revert this accidental whitespace change.
Comment 3 Nadav Rotem 2013-09-29 20:58:03 PDT
Created attachment 212947 [details]
Patch
Comment 4 Geoffrey Garen 2013-09-29 21:49:41 PDT
Comment on attachment 212947 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:660
> +    void compileAddSub(AddOrSubKind OpKind)
>      {
> +        bool isSub = OpKind == Sub;

Sorry to keep going with the style nits, but the "O" in "OpKind" should be lower case.

I'm going to r+ this patch. Can you post a follow-up to fix the "O"?
Comment 5 Nadav Rotem 2013-09-29 22:07:48 PDT
Okay. I will fix the 'O'
Comment 6 Nadav Rotem 2013-09-29 22:09:44 PDT
Created attachment 212949 [details]
Patch
Comment 7 Geoffrey Garen 2013-09-29 23:04:12 PDT
Comment on attachment 212949 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2013-09-29 23:31:40 PDT
Comment on attachment 212949 [details]
Patch

Clearing flags on attachment: 212949

Committed r156634: <http://trac.webkit.org/changeset/156634>
Comment 9 WebKit Commit Bot 2013-09-29 23:31:42 PDT
All reviewed patches have been landed.  Closing bug.