I'm afraid I don't like parts of this change. The new RegExpMacroAssemblerImpl typedef is only used in one place and that's inside an ifdef. It only adds confusion. The new include file does something very simple and is used twice where it could be inlined.
Modulo this and the other comments it LGTM http://codereview.chromium.org/12469/diff/22/204 File src/jsregexp.cc (right): http://codereview.chromium.org/12469/diff/22/204#newcode46 Line 46: #include "regexp-macro-assembler-native.h" I don't think moving this out to another file adds anything. http://codereview.chromium.org/12469/diff/22/204#newcode2583 Line 2583: RegExpMacroAssemblerImpl macro_assembler(RegExpMacroAssembler::UC16, I liked it better before (apart from the __arm__ and __thumb__ stuff). http://codereview.chromium.org/12469/diff/22/206 File src/regexp-macro-assembler-arm.cc (right): http://codereview.chromium.org/12469/diff/22/206#newcode39 Line 39: : RegExpMacroAssemblerIrregexp(NULL) {} This should have UNREACHABLE in it. http://codereview.chromium.org/12469/diff/22/210 File src/regexp-macro-assembler-native.h (right): http://codereview.chromium.org/12469/diff/22/210#newcode39 Line 39: typedef RegExpMacroAssemblerARM RegExpMacroAssemblerImpl; I don't like this at all. http://codereview.chromium.org/12469 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
