Summary: | FTL: Refactor compileArithDiv and compileArithMod into one function. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nadav Rotem <nrotem> | ||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, fpizlo, ggaren | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Nadav Rotem
2013-10-01 22:54:45 PDT
Created attachment 213153 [details]
Patch
Comment on attachment 213153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213153&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:657 > - enum AddOrSubKind {Add, Sub}; > - void compileAddSub(AddOrSubKind opKind) > + enum ArithKind {Add, Sub, Div, Mod}; Doesn’t seem helpful to share a single enum for these four kinds of arithmetic. Why not a second enum? Darin, I will split the enums and resubmit. Created attachment 213169 [details]
Patch
Comment on attachment 213169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213169&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:783 > + enum DivOrModKind {Div, Mod}; Formatting error. We use spaces inside the braces. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:787 > + bool isDiv = opKind == Div; Seems like we could say "opKind == Div" at the two sites where this is used. The boolean doesn’t add significant clarity. Created attachment 213172 [details]
Patch
Comment on attachment 213172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213172&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:294 > - compileArithDiv(); > + compileArithDivMod(Div); > break; > case ArithMod: > - compileArithMod(); > + compileArithDivMod(Mod); General comment: is there a reason why we even have an enum that we pass to these? For example, compileArithMinOrMax() doesn't take any arguments; it figures out what it should do by looking at m_node->op() - either it's ArithMin or ArithMax. Hence no need for an argument or a separate enum. What do you think about that for DivOrMod() and AddOrSub()? Created attachment 213175 [details]
Patch
Filip and Darin, thanks for the code review! Comment on attachment 213175 [details] Patch Clearing flags on attachment: 213175 Committed r156784: <http://trac.webkit.org/changeset/156784> All reviewed patches have been landed. Closing bug. |