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 _______________________________________________ Tinycc-devel mailing list [email protected] https://lists.nongnu.org/mailman/listinfo/tinycc-devel
