[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Tinycc-devel] mob broken; how to develop with mob and community
From: |
Michael Matz |
Subject: |
[Tinycc-devel] mob broken; how to develop with mob and community |
Date: |
Sat, 3 May 2014 20:44:25 +0200 (CEST) |
User-agent: |
Alpine 2.00 (LNX 1167 2008-08-23) |
Hello,
okay, are the last commits to mob from jiang meant as joke or vandalism?
* 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?
Ciao,
Michael.
- [Tinycc-devel] mob broken; how to develop with mob and community,
Michael Matz <=