Hi Slava,
Thanks for the awesome code review! I would guess this'll take me about
week or two to fix up - is that ok? N.B. I should probably reassure you
that I don't consider this finished at all, but needed the merge because
I was spending so much of my time rebasing.
Some notes;
> -template <typename T> static T *copy_untagged_object(T *untagged)
> +
> +template <typename TYPE> TYPE *factorvm::copy_untagged_object(TYPE
*untagged)
>
> Why did you rename T to TYPE? I noticed you did this in other places too.
>
Clashes with the T that is factor's 'true' (gcc barfs).
> // callback functions
> void relocate_code_block(code_block *compiled, factorvm *myvm);
> void copy_literal_references(code_block *compiled, factorvm *myvm);
> void update_word_references(code_block *compiled, factorvm *myvm);
> void update_literal_and_word_references(code_block *compiled,
factorvm *myvm);
>
> Why are these necessary?
Because you can't pass member functions as pointers. (Or maybe I'm
missing a trick here?)
> --- vm/vm-data.hpp
> +struct factorvmdata {
> + // if you change this struct, also change vm.factor k--------
>
> This is a hack. Why have factorvmdata and factorvm?
This was to help with debugging memory issues - I could switch from
struct vars to global vars with a macro switch, which was handy for the
period when master win32 wouldn't build and I wasn't sure if struct was
causing problems or not. It should go now as win32 is working.
> #define VM_ASM_API VM_C_API __attribute__ ((regparm (2)))
> #define VM_ASM_API_OVERFLOW VM_C_API __attribute__ ((regparm (3)))
>
> Make up your mind one way or another. Either use regparam(2) or
> regparm(3) for everything.
Ok, do you have any issue with using regparm(3) for all VM_ASM_API
operations? Is there a performance implication that I don't know about?
> There is a proliferation of #ifdefs in the VM struct. Notice how most
> of the rest of the source does not have platform-specific ifdefs.
I'm not 100% sure the best way to remove them from the vm struct since
the platform #includes need to go before the vm.hpp so there would be
dependency issues with using inheritance. Will have a think about this.
Thanks again,
Phil
Slava Pestov wrote:
> Hi Phil,
>
> The code quality issues I raised with primitive definitions and
> %vm-invoke-* are the most important at this stage, but I also found
> some other problems, mostly with code formatting and design. Can you
> give me an approximate time-frame for fixing these issues?
>
> --- compiler.cfg.alias-analysis
>
> M: ##vm-field-ptr insn-slot# fieldname>> 1array ; ! is this right?
>
> That works, but you don't really need the '1array' there, and you
> should rename the slot to field-name or just field.
> Wedontuseruntogetherwordsinfactor.
>
> --- compiler.codegen
>
> M: ##vm-field-ptr generate-insn
> [ dst>> ] [ fieldname>> ] bi %vm-field-ptr ;
>
> Use the CODEGEN: parsing word:
>
> CODEGEN: ##vm-field-ptr %vm-field-ptr
>
> --- cpu.x86.32
>
> On x86.32, you define a constant vm-ptr-size, but this is always 4. It
> isn't needed.
>
> --- cpu.x86.64
>
> There are some unnecessary blank lines. No more than one blank line
> between every two definitions.
>
> --- cpu.x86.64.bootstrap
>
> Unnecessary blank lines.
>
> --- cpu.x86.bootstrap
>
> Inconsistent naming. There is arg and you added arg2. Rename arg to arg1.
>
> --- cpu.x86
>
> Unnecessary blank lines.
>
> --- vm
>
> Update to use STRUCT: instead of C-STRUCT: since the latter is deprecated.
>
> --- mttest
>
> Rename to something like native-thread-test. Again, noruntogetherwordsplease.
>
> --- VM
>
> Rename factorvm to factor_vm. We have a byte_array struct, not bytearray.
>
> --- vm/arrays.cpp
>
> Unnecessary blank lines.
>
> --- vm/bignums.cpp
>
> Indentation is all screwed up in this file now. Either revert to the
> original two-space indentation
>
> if(foo)
> {
> blah
> }
>
> Or use the same indentation as the rest of the VM, with hard tabs set
> to 8 spaces
>
> if(foo)
> {
> blah
> }
>
> Not this
>
> if(foo)
> {
> blah
> }
>
> The GC_BIGNUM macro is called in all places with 'this' as the second
> argument. You can just put this in the macro.
>
> FOO_TO_BIGNUM(name,type,utype), BIGNUM_TO_FOO(name,type,utype),
> DTB_WRITE_DIGIT(factor) macros: the \ no longer line up on the right
> hand side.
>
> bignum *factorvm::bignum_multiply_unsigned_small_factor(bignum * x,
> bignum_digit_type y,int negative_p)
>
> Missing space after final comma.
>
> ---- vm/byte_arrays.hpp
>
> Various classes have a field named 'myvm'. Rename this to 'owner'.
>
> --- vm/code_block.cpp
>
> Unnecessary blank lines.
>
> in cell factorvm::relocation_offset_of(relocation_entry r):
>
> Unnecessary double space.
>
> void relocate_code_block(code_block *compiled, factorvm *myvm) -- why
> do you need this as a top-level function?
>
> code_block *factorvm::add_code_block(cell type,cell code_,cell
> labels_,cell relocation_,cell literals_)
>
> Missing spaces after commas.
>
> --- vm/code_block.hpp
>
> /* address of vm object*/
>
> Missing space before */.
>
> // callback functions
> void relocate_code_block(code_block *compiled, factorvm *myvm);
> void copy_literal_references(code_block *compiled, factorvm *myvm);
> void update_word_references(code_block *compiled, factorvm *myvm);
> void update_literal_and_word_references(code_block *compiled, factorvm *myvm);
>
> Why are these necessary?
>
> --- vm/cpu-x86.32.hpp
>
> #define VM_ASM_API VM_C_API __attribute__ ((regparm (2)))
> #define VM_ASM_API_OVERFLOW VM_C_API __attribute__ ((regparm (3)))
>
> Make up your mind one way or another. Either use regparam(2) or
> regparm(3) for everything.
>
> --- vm/cpu-x86.S
>
> -DEF(F_FASTCALL void,c_to_factor,(CELL quot)):
> +
> +DEF(F_FASTCALL void,c_to_factor,(CELL quot, void *vm)):
> PUSH_NONVOLATILE
> mov ARG0,NV_TEMP_REG
> -
>
> Why did you add a blank line in one place and remove it from another?
>
> + push ARG1 /* save vm ptr */
> call MANGLE(save_callstack_bottom)
> -
> + pop ARG1
>
> Potential stack alignment problem on Mac OS x86.32.
>
> --- vm/data_gc.hpp
>
> Unnecessary blank lines.
>
> -template <typename T> static T *copy_untagged_object(T *untagged)
> +
> +template <typename TYPE> TYPE *factorvm::copy_untagged_object(TYPE *untagged)
>
> Why did you rename T to TYPE? I noticed you did this in other places too.
>
> --- vm/image.cpp
>
> void fixup_code_block(code_block *compiled,factorvm *myvm)
>
> Missing space after comma.
>
> --- vm/inlineimpls.hpp
>
> +// I've had to copy inline implementations here to make dependencies
> work. Am hoping to move this code back into include files
> +// once the rest of the reentrant changes are done. -PD
>
> Do this ASAP.
>
> --- vm/master.hpp
>
> Unnecessary blank lines.
>
> --- vm/math.cpp
>
> - bignum * result =
> digit_stream_to_bignum(n_digits,bignum_producer,0x100,0);
> + // bignum * result =
> factor::digit_stream_to_bignum(n_digits,factor::bignum_producer,0x100,0);
> + bignum * result =
> digit_stream_to_bignum(n_digits,factor::bignum_producer,0x100,0);
>
> What happened here?
>
> +VM_C_API void box_float(float flo,factorvm *myvm) // not sure if
> this is ever called
>
> Yes, it is called. Missing space after comma.
>
> --- vm/os-windows-nt.cpp
>
> + if ((dwTlsIndex = TlsAlloc()) == TLS_OUT_OF_INDEXES) {
> + fatal_error("TlsAlloc failed - out of indexes",0);
> + }
>
> Formatting: { should be on its own line.
>
> --- vm/vm-data.hpp
> +struct factorvmdata {
> + // if you change this struct, also change vm.factor k--------
>
> This is a hack. Why have factorvmdata and factorvm?
>
> --- vm/vm.hpp
>
> There is a proliferation of #ifdefs in the VM struct. Notice how most
> of the rest of the source does not have platform-specific ifdefs.
>
> There are also a lot of ifdefs for FACTOR_REENTRANT,
> FACTOR_SINGLE_THREADED_SINGLETON, etc. All of this should go away.
>
> Slava
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> _______________________________________________
> Factor-talk mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/factor-talk
>
------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Factor-talk mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/factor-talk
