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
