From: Alexander L. <Ale...@le...> - 2000-12-13 17:39:48
|
On 12 Dec, Mark Taylor wrote: >> Date: Tue, 12 Dec 2000 17:51:32 +0100 (CET) >> From: Alexander Leidinger <Ale...@le...> >> Content-Type: TEXT/plain; charset=us-ascii >> >> >> >> ------ Forwarded message ------ >> From: Alexander Leidinger <Ale...@le...> >> Subject: ping-pong patches, round #3 >> Date: Tue, 12 Dec 2000 17:10:46 +0100 (CET) >> To: lame-dev <lam...@li...>, ma...@us... >> Message-ID: <200012121610.eBCGAmS01364@Magelan.Leidinger.net> >> >> Hi, >> >> http://www.leidinger.net/lame/ping-pong_no-testcase-change_20001212.patch >> 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 output. 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 old one. 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, right? > 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? Bye, Alexander. -- 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 |