Bug 19751 - Merge LogicalAndNode and LogicalOrNode to LogicalOpNode
Summary: Merge LogicalAndNode and LogicalOrNode to LogicalOpNode
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:19 PDT by Gavin Barraclough
Modified: 2008-07-17 05:06 PDT (History)
1 user (show)

See Also:


Attachments
Patch to merge LogicalAndNode and LogicalOrNode (8.02 KB, patch)
2008-06-24 09:19 PDT, Gavin Barraclough
zwarich: review-
Details | Formatted Diff | Diff
With new added LogicalOperator enum goodness. (7.83 KB, patch)
2008-06-25 08:09 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:19:05 PDT
Post Squirrelfish there is no longer a performance gain in having separate nodes for 'and' and 'or' – merging these will simplify & reduce code duplication in the parse tree.

Patch following.
Comment 1 Gavin Barraclough 2008-06-24 09:19:58 PDT
Created attachment 21910 [details]
Patch to merge LogicalAndNode and LogicalOrNode
Comment 2 Cameron Zwarich (cpst) 2008-06-24 21:08:57 PDT
Comment on attachment 21910 [details]
Patch to merge LogicalAndNode and LogicalOrNode

I like the idea of this patch a lot. I have been thinking of doing this for other node types, including UnaryOpNode, BinaryOpNode, and the ones you have already done.

The only thing I don't like about this patch is that you add more cases to the Operator enum. I would prefer a separate enum for OpLogicalOr and OpLogicalAnd. Change that and I'll review it.
Comment 3 Cameron Zwarich (cpst) 2008-06-24 21:37:56 PDT
Sam pointed out that I didn't suggest a good name for the new enum. LogicalOperator seems like a good choice to me.
Comment 4 Gavin Barraclough 2008-06-25 08:09:54 PDT
Created attachment 21929 [details]
With new added LogicalOperator enum goodness.
Comment 5 Cameron Zwarich (cpst) 2008-06-25 10:17:39 PDT
Comment on attachment 21929 [details]
With new added LogicalOperator enum goodness.

r=me
Comment 6 Gavin Barraclough 2008-07-17 05:06:42 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 35221.