On 12 Dec, Mark Taylor wrote:
>> Date: Tue, 12 Dec 2000 17:51:32 +0100 (CET)
>> From: Alexander Leidinger <Alexander@...>
>> Content-Type: TEXT/plain; charset=us-ascii
>> ------ Forwarded message ------
>> From: Alexander Leidinger <Alexander@...>
>> Subject: ping-pong patches, round #3
>> Date: Tue, 12 Dec 2000 17:10:46 +0100 (CET)
>> To: lame-dev <lame-dev@...>, mart@...
>> Message-ID: <200012121610.eBCGAmS01364@...>
>> is another set of patches (tested with "make test" and something similar
>> for vbr).
> This patch has a few good changes, along with adding some insulting
> comments about my coding, and many possibly dangerious
> but surely pointless changes.
I just tried to bring back backed out changes which didn't change the
BTW.: roberts vbr-mtrh diff does change the output of make test (from
595 to 300), but only for me, not for him. Can you please test it too?
> Here's a quick list:
> init_outer_loop: return code is changed from sum>0 to
> sum>1e-20. Unless someone has computed that energy
> values < 1e-20 will *never* be quanitzed to non zero values,
> this is introduce a bug just because someone wanted to pointless
> changes to a 100% fully working routine that I wrote.
Look into the for loop. As I understand it, the behavior is the same,
but someone has replaced 576 conditionals with just one conditional
(with the assumtion that 576 conditionals + 576 integer adds are slower
than 576 floating point adds + one conditional).
> Logic in reservoir.c is changed. I'm sure no bugs were fixed,
No, the logic is the same, just the implementation of the logic has
changed. I didn't just applied the patch, I readded it to the source by
hand and tried to make sure, it didn't changes the behavior.
> but by changing this we run the risk of introducing new bugs.
> New code is no clearer than my original code. Why change this?
That's your opinion. After doing the merge by hand and ensuring that it
hasn't changed the logic, I have to say I like the new one more than the
Yes, the old one worked, but the new one is "more correct", or better:
it gives more hints about the code flow.
E.g. in the old code, you didn't know if the initial value of
gfc->ResvMax can be set at another point in the code, with the new code
you have the implicit information: "The initial value of gfc->ResvMax
gets set only here".
The name of this variable also suggests, that it is a variable which is
only initialised in this function, but this is speculation, I haven't
looked at it that closely.
That's IMHO an improvement, and improvements are good by definition,
> The 6 line, bug free ms_convert() is rewritten. I like the
> version that I wrote better. Why would someone else feel
> the need to change this?
No, it isn't rewritten, there was just a name change from xr to dst and
from xr_org to src. This gives more information to the reader than xr
and xr_org. The move of "FLOAT8 ..." can be an optimization, depending
on the compiler, but I hope todays compilers know how to optimize this
on their own. You may flag this as an "cosmetic change", I think it's an
improvement of the readability of the code.
> tables.c, psymodel.c: fills the code with more #define KLEMM's,
> just so he can replace .33333 by 1.0/3.0 in the tables,
He has additional changes for this piece of code, but because you didn't
want to change the actual tables, he has to use a copy of them,
protected by a define. The end result will either be faster (speed),
cleaner (code) and/or better (quality). He produces an infrastructure on
which he want's to improve various things. We can't do listening tests
on not written code, and maintaining a huge diff is very time consuming,
time which is better spend on improving lame.
Perhaps Frank jumps in and explaines more about it.
The code is propperly protected by a define which isn't on by default
(as requested by you in another message to the mailinglist).
> and rewrite the simple, clear, bug free, table reading code.
> This is the worst kind of pointless code motion.
At the moment it's just code motion, but moving the encoding source into
libmp3lame/ was also "pointless code motion", at least in the beginnig.
Now we have a libmp3lame.a (and sometimes in the future libmp3lame.so).
Can you please have a closer look at those "pointless changes" again?
Give a man a fish and you feed him for a day;
teach him to use the Net and he won't bother you for weeks.
http://www.Leidinger.net Alexander @ Leidinger.net
GPG fingerprint = 7423 F3E6 3A7E B334 A9CC B10A 1F5F 130A A638 6E7E