Re: [Tinycc-devel] mob broken; how to develop with mob and community

[Ramsay Jones]

On 03/05/14 19:44, Michael Matz wrote:
> Hello,
> 
> okay, are the last commits to mob from jiang meant as joke or vandalism?
> 

I would like to think it wasn't meant as vandalism, but it certainly
looks like vandalism to me (at least to the i386 build, which currently
doesn't even compile!). :(

> * 32bit code generation is hosed already in the testsuite, * gawk doesn't 
> work anymore even for x86_64,
> * arm codegen is broken already in the testsuite (adding an internal
>   compiler error)
> * they contain ugly white-space changes making review exceedingly hard
> * despite the unnecessarily hard review I think there are numerous
>   problems in the actual implementation:
>   + the new parse_number uses inexact floating point directly (e.g. 1.0L/b
>     when b==10 isn't exactly representable, cumulating errors while
>     parsing)
>   + There's a new subtype VT_VLS meaning VLA plus STRUCT, which makes no
>     sense at all (VLA is VL _array_)
>   + TREG_MEM (also new) doesn't follow convention for type flags, and
>     seems like a layering violation
>   + It reverts a cleanup by Thomas (eda2c756edc4dca004ba217) without
>     discussion
>   + It renames libtcc1.a to libcrt.a, thereby trading a sensibly
>     tcc-specific name for something tcc-specific with something generic
>     (what if gcc had libcrt as well?)
>   + It increases VT_STRUCT_SHIFT to 20, breaking bitfields larger than 31
>     bits (we needs 12 bits to encode bitfield position and size, so the
>     maximum bit shift can be 19
>   + It changes gv2() so that VT_CMP/VT_JMP results aren't special-cased
>     anymore, without obvious compensation in all its users to avoid the
>     errors that the comment specifically mentioned
>   + It implements some strange non-standard preprocessor extension
>     push_macro/pop_macro (as pragmas) without discussion; it enlargens
>     some heavily used internal data structures for this.
>   + It adds some "fix x86-64 vla" commit, without testcase showing what's
>     actually broken, and for that shuffles the internal code generations
>     in large and unobvious ways (and removes the correct calls to alloca()
>     on x86-64 PE)
> 
> And that's just what I saw on a cursory read of the commits.  Due to the 
> white-space changes the more intricate parts are terrible to review and I've 
> skipped them.
> 
> When I wrote above "without discussion", then this was just for the most 
> controversial parts.  It's true for all the patches.  I've seen no messages 
> at all from jiang to this mailing list.  No discussion about implementation 
> approaches, no discussions about bugs, no nothing.  The commit messages are 
> mostly non-informative as well.
> 
> All in all I think this approach is pretty unacceptable, but others here 
> might differ.  If the patch series were a smaller then the problems in it 
> could reasonably be fixed after the fact by others.  But as it stands we now 
> have something in which every single one of the 22 topmost patches (ignoring 
> the white-space fixup patch from grischka) has issues.
> 
> If it were just my project I'd be tempted to revert the whole mob state to be 
> before your (jiangs) patches, and expect you to work with the community to 
> fix what you actually wanted to fix or improve.  (From the patch series I 
> gather that one thing you wanted to fix was parameter passing on stack when 
> memcpy is needed).  It the very minimum you have to subscribe to this mailing 
> list (that's even listed in the mob rules), and of course also take part in 
> discussions.  You also have to _review_ your patches before commiting (you 
> would have seen the useless white-space changes) and write meaningful commit 
> messages.
> 
> Any opinions from others?

If it were my project, I would have reverted all of those commits 
some time ago! (NOTE: I haven't made significant contributions to
this project, so I don't get a vote on this).

ATB,
Ramsay Jones