Forwarding, subscribed to v8-dev :) ---------- Forwarded message ---------- From: <[email protected]> Date: Fri, Aug 27, 2010 at 10:27 AM Subject: Re: Remove dependence of code-stubs on codegen, the virtual frame code generator.... (issue3169049) To: [email protected] Cc: [email protected]
My concern is making the header-file dependencies more tangled than they already are. Normally we have platform-independent interface .h files include the appropriate platform specific parts rather than the other way around or keeping them separate (as far as I can see, the regexp stuff is the exception not the rule). If you're sure all the new includes are needed (and can't be replaced with a few forward declarations) in header files, then LGTM. http://codereview.chromium.org/3169049/diff/11002/39001 File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/3169049/diff/11002/39001#newcode33 src/arm/code-stubs-arm.cc:33: #include "code-stubs-arm.h" "code-stubs.h"? I think normally we list our own header file first (but after v8.h). http://codereview.chromium.org/3169049/diff/11002/39002 File src/arm/code-stubs-arm.h (right): http://codereview.chromium.org/3169049/diff/11002/39002#newcode31 src/arm/code-stubs-arm.h:31: #include "ast.h" As far as I can see, this file doesn't use anything from ast.h. Please don't include it here (it brings in a lot of stuff including assembler and codegen stuff, so this risks circularity). Where it is needed is in regexp-macro-assembler.h which is included in code-stubs-arm.cc. Please just include ast.h in regexp-macro-assembler.h. (I seem to remember trying to just forward declare the regexp AST classes in regexp-macro-assembler but it didn't trivially work because of hyper-aggressive inlining. We should consider having a separate regexp ast module.) http://codereview.chromium.org/3169049/diff/11002/39002#newcode32 src/arm/code-stubs-arm.h:32: #include "code-stubs.h" This one seems weird too. Normally, code-stubs.h would include code-stubs-arm.h on ARM (not the other way around). That way a programmer just has to include code-stubs.h to get both the platform-independent and dependent parts of the module. Please see macro-assembler.h as an example. http://codereview.chromium.org/3169049/diff/11002/39002#newcode34 src/arm/code-stubs-arm.h:34: #include "type-info.h" Is this one really needed? I don't see where. http://codereview.chromium.org/3169049/diff/11002/39004 File src/arm/codegen-arm.h (right): http://codereview.chromium.org/3169049/diff/11002/39004#newcode32 src/arm/codegen-arm.h:32: #include "code-stubs-arm.h" "code-stubs.h" http://codereview.chromium.org/3169049/diff/11002/39005 File src/arm/regexp-macro-assembler-arm.cc (right): http://codereview.chromium.org/3169049/diff/11002/39005#newcode39 src/arm/regexp-macro-assembler-arm.cc:39: #include "arm/macro-assembler-arm.h" No need for this one. http://codereview.chromium.org/3169049/diff/11002/39005#newcode41 src/arm/regexp-macro-assembler-arm.cc:41: #include "arm/code-stubs-arm.h" Also can get rid of this one by including the appropriate platform-specific interface in code-stubs.h. http://codereview.chromium.org/3169049/diff/11002/39007 File src/code-stubs.h (right): http://codereview.chromium.org/3169049/diff/11002/39007#newcode119 src/code-stubs.h:119: static Major getMajorKey(Code* code_stub) { What the heck is this naming convention? get_major_key or GetMajorKey. http://codereview.chromium.org/3169049/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
