Re: ORBit2 status: Workaround for hang, but new alignment problems

Hi Tor,

	So - I think we should CC the ORBit list for all this (great) work
porting to Win32.

	I have a number of comments / suggestions, but it's looking good
overall:


+ general:
	+ offset: should use 'ALIGN_VALUE' not ALIGN_ADDRESS.
		+ get rid of the pointer kniptions :-)
			offset = ALIGN_VALUE (offset, tc...);
			*val = val0 + offset;
	+ struct:
		+ the separate offset += ORBit_gather_alloc_info:
			+ rather slow; I liked the pre-align there
				+ perhaps add a 'ulong &size' argument
				  to marshal_value in due course (?)
	+ not convinced by tk_union
		+ we remove a pre-align, but I don't see an equivalent
		  post-align:

	-		discrim = *val = ALIGN_ADDRESS (*val, MAX
(tc->discriminator->c_align, tc->c_align));
	+		discrim = *val;

		+ unions are nasty & complex ;-)

	+ in _marshal_value it seems to make little sense to
	  do the GIOP & native alignment at different ends :-)
	  can we bin all pre-aligns in favour of post ?

	Finally, in the tests; I have a suspicion that on Linux & Win32 we are
prolly loosing some useful alignment assistance by not having:
	
	volatile guchar dummy = <value>;

	members between structures on the stack (the volatile may/should force
the optimiser to think twice about whacking them in registers / removing
them altogether).

	Apart from that this looks good, I havn't tested it on anything yet, I
guess I'd like to read the patch again with the above massage into
place; and then run it - it'd be nice to be sure it was 100% correct by
code reading before testing it anywhere :-)

	I'm hoping we can also use this code for some GType work that we really
need to push this sort of thing down into glib itself in due course.

	Thanks,

		Michael.

-- 
 [email protected]  <><, Pseudo Engineer, itinerant idiot



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click