Bug 19750 - Merge inc/dec halves of PrefixResolveNode and PostfixResolveNode
Summary: Merge inc/dec halves of PrefixResolveNode and PostfixResolveNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-24 09:16 PDT by Gavin Barraclough
Modified: 2008-07-17 06:26 PDT (History)
1 user (show)

See Also:


Attachments
Patch to merge inc/dec halves of PrefixResolveNode and PostfixResolveNode (13.28 KB, patch)
2008-06-24 09:16 PDT, Gavin Barraclough
zwarich: review-
Details | Formatted Diff | Diff
No method pointers, fewer parentheses, fixed fixme. (13.48 KB, patch)
2008-06-25 08:13 PDT, Gavin Barraclough
zwarich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2008-06-24 09:16:02 PDT
Post Squirrelfish there is no longer a performance gain in having separate nodes for inc and dec – merging these will simplify & reduce code duplication in the parse tree.

Patch following.
Comment 1 Gavin Barraclough 2008-06-24 09:16:44 PDT
Created attachment 21908 [details]
Patch to merge inc/dec halves of PrefixResolveNode and PostfixResolveNode
Comment 2 Cameron Zwarich (cpst) 2008-06-24 21:29:36 PDT
Comment on attachment 21908 [details]
Patch to merge inc/dec halves of PrefixResolveNode and PostfixResolveNode

I think this is unnecessary:

+    RegisterID* (CodeGenerator::*emitPostIncOrDec)(RegisterID* dst, RegisterID* srcDst) = (m_operator == OpPlusPlus) ? &CodeGenerator::emitPostInc : &CodeGenerator::emitPostDec;

+    RegisterID* (CodeGenerator::*emitPreIncOrDec)(RegisterID* srcDst) = (m_operator == OpPlusPlus) ? &CodeGenerator::emitPreInc : &CodeGenerator::emitPreDec;

Use CodeGenerator::emitUnaryOp() and CodeGenerator::emitUnaryNoDstOp() with an inline conditional expression, or make a helper function.

You don't need parentheses around the entire conditional expression here:

+            RefPtr<RegisterID> r0 = generator.emitLoad(generator.finalDestination(dst), ((m_operator == OpPlusPlus) ? 1.0 : -1.0));

Also, rewrite the FIXME comment in PostfixResolveNode::emitCode() so it is still correct.
Comment 3 Gavin Barraclough 2008-06-25 08:13:17 PDT
Created attachment 21930 [details]
No method pointers, fewer parentheses, fixed fixme.


Cameron,

I've replaced the method pointers with simple static helper methods in nodes.cpp – if you could have a look & see what you think that'd be great.

cheers,
G.
Comment 4 Cameron Zwarich (cpst) 2008-06-25 10:29:28 PDT
Comment on attachment 21930 [details]
No method pointers, fewer parentheses, fixed fixme.

r=me
Comment 5 Gavin Barraclough 2008-07-17 06:26:17 PDT
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/kjs/grammar.y
Sending        JavaScriptCore/kjs/nodes.cpp
Sending        JavaScriptCore/kjs/nodes.h
Sending        JavaScriptCore/kjs/nodes2string.cpp
Transmitting file data .....
Committed revision 35222.